Switch to clang-format #410

julianoes · 2018-05-24T20:51:06Z

While the clang-format style tries to reflect the previous astyle style
as close as possible, there is, unfortuntately, still a considerable diff.

Fixes #358.

JonasVautherin

Fine for me as soon as the tests pass!

hamishwillee · 2018-05-28T06:37:19Z

@julianoes We mention astyle here: https://docs.dronecore.io/en/contributing/code_style.html

Obviously we're now using clang.

What are the style settings defined by (previously it said astyle settings defined by astylerc).
Is it still called by make fix_style?
Are the explanations accurate? (other than typo "Large enough TO allow ...)

I will have to remove astyle from the dependencies - e.g. : https://docs.dronecore.io/en/contributing/build.html#linux
What do have to add for Clang?

JonasVautherin · 2018-05-29T19:41:34Z

core/mavlink_parameters.cpp

@@ -111,7 +111,7 @@ MAVLinkParameters::do_work()
        _state = State::SET_PARAM_BUSY;

        char param_id[PARAM_ID_LEN] = {};
-        STRNCPY(param_id, set_param_work->param_name.c_str(), sizeof(param_id));
+        STRNCPY(param_id, set_param_work->param_name.c_str(), sizeof(param_id)-1);


I don't get that :-/

Oh, forget it, that's in the commit description 😅

GCC 8 doesn't like the fact that we could potentially copy 17 chars without null termination into param_id. At that point param_id would not be null terminated. That's how I understand it.

I was just thinking, why don't we a hire a local std::string for doing this. Applicable elsewhere in such scenarios as well. Makes sense ?

@shakthi-prashanth-m I don't think I understand what you mean.

julianoes · 2018-05-29T19:59:51Z

@hamishwillee
@julianoes We mention astyle here: https://docs.dronecore.io/en/contributing/code_style.html

Yes, we are switching.

Obviously we're now using clang.

What are the style settings defined by (previously it said astyle settings defined by astylerc).

The settings are defined in .clang-format (added in 7269ce5)

Is it still called by make fix_style?

Yes, the usage is the same.

Are the explanations accurate? (other than typo "Large enough TO allow ...)

Yes, that's still the same.

I will have to remove astyle from the dependencies - e.g. : https://docs.dronecore.io/en/contributing/build.html#linux
What do have to add for Clang?

Correct. Developers need to have clang-format version >= 6.

For Ubuntu 16.04: (https://github.com/dronecore/DroneCore/blob/switch-to-clang-format/docker/Dockerfile-Ubuntu-16.04#L37-L42)

wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | sudo apt-key add -
sudo apt-add-repository "deb http://apt.llvm.org/xenial/ llvm-toolchain-xenial-6.0 main"
sudo apt-get update
sudo apt-get install -y clang-format-6.0
sudo update-alternatives --install /usr/bin/clang-format clang-format /usr/bin/clang-format-6.0 1000

Ubuntu 18.04: (https://github.com/dronecore/DroneCore/blob/switch-to-clang-format/docker/Dockerfile-Ubuntu-18.04#L21)

sudo apt-get install clang-format
sudo update-alternatives --install /usr/bin/clang-format clang-format /usr/bin/clang-format-6.0 1000

Fedora 28: (https://github.com/dronecore/DroneCore/blob/switch-to-clang-format/docker/Dockerfile-Fedora-28#L14)

sudo dnf install clang

shakthi-prashanth-m

So many new changes 👍 Code looks clean now :-)

shakthi-prashanth-m · 2018-05-30T11:52:30Z

plugins/telemetry/telemetry_impl.cpp

-    _ground_speed_ned_rate_hz(0.0),
-    _position_rate_hz(-1.0)
+TelemetryImpl::TelemetryImpl(System &system)
+    : PluginImplBase(system), _position_mutex(),


At some places like these (constructor member initialization), I feel old style is better readable. But, thats unavoidable I think.

Good call, I was going to look at the changes and figure out how to prevent some of them.

This is an attempt to come up with the config for clang-format 6 which matches our current format as close as possible. Some options for clang-format 7 are currently commented out.

The destination needs to be 1 char longer than the num of chars that are copied in order to make sure the resulting string at the destination is null-terminated.

This means that we can generate the binaries for multiple distro versions.

We switched from astyle to clang-format. Even though we tried to match the previous style as much as possible, there are still a few changes needed.

julianoes · 2018-05-30T15:00:14Z

@shakthi-prashanth-m @JonasVautherin I changed some of the clang-format options to match the previous style closer. Have another look please if you see anything too ugly.

JonasVautherin

Looks good to me!

Switch to clang-format

julianoes requested a review from shakthi-prashanth-m May 24, 2018 20:51

julianoes added the in progress label May 24, 2018

JonasVautherin approved these changes May 25, 2018

View reviewed changes

julianoes force-pushed the switch-to-clang-format branch from 1a3b383 to bba0984 Compare May 25, 2018 12:49

JonasVautherin approved these changes May 29, 2018

View reviewed changes

JonasVautherin reviewed May 29, 2018

View reviewed changes

shakthi-prashanth-m approved these changes May 30, 2018

View reviewed changes

julianoes force-pushed the switch-to-clang-format branch from 0282e81 to a8013ca Compare May 30, 2018 13:43

julianoes and others added 17 commits May 30, 2018 10:54

Added .clang-format matching our current style

2451d20

This is an attempt to come up with the config for clang-format 6 which matches our current format as close as possible. Some options for clang-format 7 are currently commented out.

fix_style.sh: switched from astyle to clang-format

47db670

build_system: sort ubuntu deps alphabetically

f7a9df5

build_system: add clang-format to ubuntu

3ee1daf

build_system: sort fedora deps alphabetically

51cc4d4

build_system: replace astyle by clang-format on fedora

2e1a90b

docker: Fedora needs clang for clang-format

3f82992

docker: remove astyle for good

6ed7ff0

docker: install clang-format-6.0 in Ubuntu 16.04

749d57b

docker: download clang-format 6 for Fedora 27

2e5cc10

docker: added Dockerfile for Fedora 28

d0ee879

docker: added Dockerfile for Ubuntu 18.04

ab4cb0d

Added Fedora 28 and Ubuntu 18.04 to CI

8588c1e

core: fix GCC 8 warning

8db1f17

The destination needs to be 1 char longer than the num of chars that are copied in order to make sure the resulting string at the destination is null-terminated.

create_packages.sh: don't forget camera header

6a32373

create_packages.sh: add distro version to filename

4673978

This means that we can generate the binaries for multiple distro versions.

Ran make fix_style with new clang-format

6db58c5

We switched from astyle to clang-format. Even though we tried to match the previous style as much as possible, there are still a few changes needed.

julianoes force-pushed the switch-to-clang-format branch from a8013ca to 6db58c5 Compare May 30, 2018 14:59

JonasVautherin approved these changes May 30, 2018

View reviewed changes

julianoes merged commit 3fcc9da into develop May 30, 2018

julianoes deleted the switch-to-clang-format branch May 30, 2018 16:20

julianoes mentioned this pull request May 30, 2018

Rendering of reference and pointer parameters in markdown script #321

Closed

mrpollo mentioned this pull request Jun 5, 2018

v1.0.0 Changelog scratchpad #417

Closed

rt-2pm2 pushed a commit to rt-2pm2/DronecodeSDK that referenced this pull request Nov 27, 2018

Merge pull request mavlink#410 from dronecore/switch-to-clang-format

18b2413

Switch to clang-format

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to clang-format #410

Switch to clang-format #410

julianoes commented May 24, 2018 •

edited by JonasVautherin

Loading

JonasVautherin left a comment

hamishwillee commented May 28, 2018

JonasVautherin May 29, 2018

JonasVautherin May 29, 2018

julianoes May 29, 2018

shakthi-prashanth-m May 30, 2018

julianoes May 30, 2018

julianoes commented May 29, 2018 •

edited

Loading

shakthi-prashanth-m left a comment

shakthi-prashanth-m May 30, 2018

julianoes May 30, 2018 •

edited

Loading

julianoes commented May 30, 2018

JonasVautherin left a comment

Switch to clang-format #410

Switch to clang-format #410

Conversation

julianoes commented May 24, 2018 • edited by JonasVautherin Loading

JonasVautherin left a comment

Choose a reason for hiding this comment

hamishwillee commented May 28, 2018

JonasVautherin May 29, 2018

Choose a reason for hiding this comment

JonasVautherin May 29, 2018

Choose a reason for hiding this comment

julianoes May 29, 2018

Choose a reason for hiding this comment

shakthi-prashanth-m May 30, 2018

Choose a reason for hiding this comment

julianoes May 30, 2018

Choose a reason for hiding this comment

julianoes commented May 29, 2018 • edited Loading

shakthi-prashanth-m left a comment

Choose a reason for hiding this comment

shakthi-prashanth-m May 30, 2018

Choose a reason for hiding this comment

julianoes May 30, 2018 • edited Loading

Choose a reason for hiding this comment

julianoes commented May 30, 2018

JonasVautherin left a comment

Choose a reason for hiding this comment

julianoes commented May 24, 2018 •

edited by JonasVautherin

Loading

julianoes commented May 29, 2018 •

edited

Loading

julianoes May 30, 2018 •

edited

Loading