Automate C++ include file grouping and ordering using clang-format #4205
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…all files, and fix some uncovered missing includes
harrism
added
improvement
github-actions
bot
added
cuGraph
and removed
improvement
harrism
added
improvement
seunghwak
approved these changes
Feb 29, 2024
| #include <c_api/error.hpp> | ||
| #include <c_api/resource_handle.hpp> | ||
| #include "c_api/array.hpp" | ||
|
|
There was a problem hiding this comment.
Why do we have an empty line here?
There was a problem hiding this comment.
There were a small number of cases like this in cudf, too. We suspected a clang-format bug was responsible. (We agreed that an automated solution, even if imperfect, is much better than manual enforcement.)
| @@ -14,14 +14,15 @@ | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| #include <cugraph_c/graph_functions.h> | |||
| #include "c_api/graph_functions.hpp" | |||
|
|
|||
There was a problem hiding this comment.
Why do we have an empty line here?
| @@ -14,13 +14,13 @@ | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| #include <utilities/device_comm_wrapper.hpp> | |||
| #include <utilities/test_utilities.hpp> | |||
| #include "c_api/mg_test_utils.h" | |||
There was a problem hiding this comment.
Shouldn't we group this header with the c_api headers below?
|
Thank you. It is super useful. |
naimnv
approved these changes
Feb 29, 2024
ChuckHastings
approved these changes
Feb 29, 2024
|
/merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This uses the
IncludeCategoriessettings in.clang-formatto automate include ordering and grouping and to make include ordering more consistent with the rest of RAPIDS. For discussion, see rapidsai/cudf#15063. This PR uses a similar set of header grouping categories used in that PR, adapted for cuGraph.One purpose of this is to make it easier to automate injection of a header change with an upcoming RMM refactoring (and in the future).
Note that this PR also updates all of cugraph to use quotes ("") when includeing local source headers. That is, whenever something is included from
cugraph/src/*, it should be included with quotes rather than angle brackets. This makes it much easier to order includes from "nearest to farthest", and matches the approach now taken in other RAPIDS repos. Without switching to quotes, keeping these includes at the top requires special casing each subdirectory ofsrcin the clang-format group regular expressions.Includes from the
includedirectory stay as angle brackets, because they always use#include <cugraph/...>.cuGraph tests include a LOT of internal source headers. I think we should generally avoid this, but it depends on the testing philosophy. If one believes that tests should only cover the external interfaces of the library, then there is no need to include internal headers. But if we want to test internal functionality, then the tests need to be more "internal" to the library.
The header reordering in this PR also uncovered some places where headers were not included where they are used, which I have fixed.
Closes #4185