Reorder and remove unused #includes #176
Conversation
kittobi1992
force-pushed
the
clang_format_revisited
branch
3 times, most recently
from
4b36772 to
a0fe818
Compare
| @@ -28,17 +28,16 @@ | |||
|
|
|||
| #include "static_graph.h" | |||
|
|
|||
| #include <algorithm> | |||
|
|
|||
| #include "tbb/parallel_reduce.h" | |||
There was a problem hiding this comment.
I think the library includes should stay with <>. Just saw this now. Will do a proper read-through later.
There was a problem hiding this comment.
nope most of them were included in this style before. Btw, I will not go over all files again. This was the most annoying work I've ever done!!! Meaning this is a eat or die change :-D
There was a problem hiding this comment.
Well, both are used very inconsistently at the moment. I don't really have a preference, but this PR might be a good opportunity to do it uniformly everywhere
There was a problem hiding this comment.
Yes, they are uniform now 😊
There was a problem hiding this comment.
btw. we have to transistion to a include-what-you-use policy, otherwise reordering #include becomes a real pain.
There was a problem hiding this comment.
Yeah, include-what-you-use definitively makes sense
There was a problem hiding this comment.
Yes, they are uniform now 😊
Are you sure? There still seem to be some TBB includes using <>, though the majority uses""
|
The updated formatting options look good to me |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #176 +/- ##
==========================================
- Coverage 78.90% 78.87% -0.03%
==========================================
Files 202 202
Lines 19632 19629 -3
Branches 8036 8035 -1
==========================================
- Hits 15490 15483 -7
- Misses 4142 4146 +4 ☔ View full report in Codecov by Sentry. |
kittobi1992
force-pushed
the
clang_format_revisited
branch
from
a0fe818 to
0528632
Compare
There's a couple options that are only supported starting v17 of clang, which is quite recent (truly latest version is v18). On Arch Linux, v16 is the latest available from the package manager (they're super quick usually), and v17 is available from the user repositories starting just two days ago. Below is a list of the unknown options for me. I'd actually really like to have the SpacesInParens option because that one is quite inconsistent in our code base. We can put these rules in, but some folks will just not be able to apply them (there's a flag to ignore unknown keys, so the other rules can be applied). Still, if we enforce the code style in a CI, we're cutting off contributions. mt-kahypar/.clang-format:213:1: warning: unknown key 'RemoveParentheses' |
|
Weirdly enough...The SpacesInParens option is warned about as unknown but it is being applied.
|
|
Installation of the newer version worked for me via the script on https://apt.llvm.org/llvm.sh (Ubuntu). But yeah, its definitively a hurdle for contributors
|
|
Thanks for the pointer -- The AUR also has v17. And I agree, the hurdle is a bit too high. Thoughts on adding a github action that will format the code-base pre-merge?
|
That's probably the best option until version 17 is more widely available |
|
I do not understand why the hurdle is too high. Contributors do not have to care about formatting unless they want to push something to master. CI tells them that their code is not formatted correctly. They run a script (that will be in our repo) that installs clang-format-17, and then a script that runs the formatter. More active contributor can also install clang-format-17 via the script, and then integrate it into their IDE. |
|
How would a generic clang-format install work -- keeping in mind not everyone uses apt (Debian/Ubuntu). If we let the CI do the formatting and let it do a commit, we can bypass this issue. See the following link for a snippet how to trigger formatting and auto-commit for a pull request. https://mskelton.medium.com/auto-formatting-code-using-prettier-and-github-actions-ed458f58b7df
|
|
The CI should be not responsible for formatting since there are some rare cases where formatting can break a build. Users on Ubuntu can use the installation script. Mac users are already on v17, and Windows, well... 😀 |
Arch Linux is on v16 -- not as bleeding edge as they like to claim. Other distros -- who knows... |
This change reorders all includes according to the definition in our new
.clang-formatfile, and removes most of the unused includes.