search

ament / uncrustify_vendor

CMake shim over the uncrustify library: https://github.com/uncrustify/uncrustify
Apache License 2.0
0 stars 9 forks source link

Update to Uncrustify v0.73.0 #14

Closed mjbogusz closed 2 years ago

mjbogusz commented 3 years ago

As the title states, update to the latest upstream release.

Ref #13

hidmic commented 3 years ago

Let's see what CI says (only uncrustify tests):

mjbogusz commented 3 years ago

I'm not sure how to interpret this. I see some minor whitespace changes and few return;-s removed, but as they're printed without context I'm not sure whether that's a good thing or not.

I'm assuming that the CI uses this config, right? I think some minor tweaks to the config would remove most of the differences that popped out.

hidmic commented 3 years ago

I see some minor whitespace changes and few return;-s removed, but as they're printed without context I'm not sure whether that's a good thing or not.

Consider reproducing locally a representative subset of these new issues (two cases, it seems) and posting the diffs in a gist. Then we can see if/how we should change our default config.

I'm assuming that the CI uses this config, right?

Default config, correct.

mjbogusz commented 3 years ago

I've rebased this PR and updated the bundled uncrustify to the latest upstream, 0.73.0. I've also pushed a PR to ament_lint with related configuration changes: https://github.com/ament/ament_lint/pull/319

I've tested this with few packages that were previously failing in CI and while it reports some problems, I think they might be legitimate, like double-indented lambda bodies - let me know how should I approach this.

hidmic commented 3 years ago

I've tested this with few packages that were previously failing in CI and while it reports some problems, I think they might be legitimate, like double-indented lambda bodies - let me know how should I approach this.

@mjbogusz since we cannot break CI for all affected downstream packages, we'll have to update them all before merging this. If the linter tests pass after applying the necessary fixes while using the current uncrustify version, we can do it incrementally. Otherwise, all PRs will have to be merged at once.

clalancette commented 2 years ago

Closing this in favor of #27. The reason that is upgrading to uncrustify 0.72 (and not a later version) is because Ubuntu 22.04 is currently using 0.72.

mjbogusz commented 2 years ago

Closing this in favor of #27. The reason that is upgrading to uncrustify 0.72 (and not a later version) is because Ubuntu 22.04 is currently using 0.72.

Seems reasonable - there were many bugfixes and improvements since 0.69 that are present in 0.72, while the two newer versions are mostly minor features and configuration splits (e.g. separately configurable sub-cases of a configuration flag).