search

kahypar / mt-kahypar

Mt-KaHyPar (Multi-Threaded Karlsruhe Hypergraph Partitioner) is a shared-memory multilevel graph and hypergraph partitioner equipped with parallel implementations of techniques used in the best sequential partitioning algorithms. Mt-KaHyPar can partition extremely large hypergraphs very fast and with high quality.
MIT License
126 stars 26 forks source link

Reorder and remove unused #includes #176

Open kittobi1992 opened 11 months ago

kittobi1992 commented 11 months ago

This change reorders all includes according to the definition in our new .clang-format file, and removes most of the unused includes.

N-Maas commented 11 months ago

The updated formatting options look good to me

codecov[bot] commented 11 months ago

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (d784c91) 78.90% compared to head (0528632) 78.87%.

Files Patch % Lines
mt-kahypar/io/hypergraph_factory.cpp 0.00% 1 Missing :warning:
mt-kahypar/partition/partitioner.cpp 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

larsgottesbueren commented 11 months ago

The updated formatting options look good to me

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' RemoveParentheses: Leave ^~~~~ mt-kahypar/.clang-format:31:1: warning: unknown key 'AlignConsecutiveShortCaseStatements' AlignConsecutiveShortCaseStatements: ^~~~~~~~~~~ mt-kahypar/.clang-format:231:1: warning: unknown key 'SpaceBeforeJsonColon' SpaceBeforeJsonColon: false ^~~~~~~~ mt-kahypar/.clang-format:184:1: warning: unknown key 'KeepEmptyLinesAtEOF' KeepEmptyLinesAtEOF: false ^~~~~~~ mt-kahypar/.clang-format:252:1: warning: unknown key 'SpacesInParens' SpacesInParens: Never ^~~~~~ mt-kahypar/.clang-format:253:1: warning: unknown key 'SpacesInParensOptions' SpacesInParensOptions: ^~~~~ mt-kahypar/.clang-format:266:1: warning: unknown key 'VerilogBreakBetweenInstancePorts' VerilogBreakBetweenInstancePorts: true

larsgottesbueren commented 11 months ago

Weirdly enough...The SpacesInParens option is warned about as unknown but it is being applied.

The updated formatting options look good to me

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' RemoveParentheses: Leave ^~~~~ mt-kahypar/.clang-format:31:1: warning: unknown key 'AlignConsecutiveShortCaseStatements' AlignConsecutiveShortCaseStatements: ^~~~~~~~~ mt-kahypar/.clang-format:231:1: warning: unknown key 'SpaceBeforeJsonColon' SpaceBeforeJsonColon: false ^~~~~~ mt-kahypar/.clang-format:184:1: warning: unknown key 'KeepEmptyLinesAtEOF' KeepEmptyLinesAtEOF: false ^~~~~ mt-kahypar/.clang-format:252:1: warning: unknown key 'SpacesInParens' SpacesInParens: Never ^~~~ mt-kahypar/.clang-format:253:1: warning: unknown key 'SpacesInParensOptions' SpacesInParensOptions: ^~~~~ mt-kahypar/.clang-format:266:1: warning: unknown key 'VerilogBreakBetweenInstancePorts' VerilogBreakBetweenInstancePorts: true

N-Maas commented 11 months ago

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

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' RemoveParentheses: Leave ^~~~~ mt-kahypar/.clang-format:31:1: warning: unknown key 'AlignConsecutiveShortCaseStatements' AlignConsecutiveShortCaseStatements: ^~~~~~~~~ mt-kahypar/.clang-format:231:1: warning: unknown key 'SpaceBeforeJsonColon' SpaceBeforeJsonColon: false ^~~~~~ mt-kahypar/.clang-format:184:1: warning: unknown key 'KeepEmptyLinesAtEOF' KeepEmptyLinesAtEOF: false ^~~~~ mt-kahypar/.clang-format:252:1: warning: unknown key 'SpacesInParens' SpacesInParens: Never ^~~~ mt-kahypar/.clang-format:253:1: warning: unknown key 'SpacesInParensOptions' SpacesInParensOptions: ^~~~~ mt-kahypar/.clang-format:266:1: warning: unknown key 'VerilogBreakBetweenInstancePorts' VerilogBreakBetweenInstancePorts: true

larsgottesbueren commented 11 months ago

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?

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

N-Maas commented 11 months ago

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

kittobi1992 commented 11 months ago

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.

larsgottesbueren commented 11 months ago

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

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.

kittobi1992 commented 11 months ago

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... 😀

larsgottesbueren commented 11 months ago

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...