Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automate C++ include file grouping and ordering using clang-format #1345

Closed
Tracked by #20
harrism opened this issue Feb 21, 2024 · 0 comments · Fixed by #1349
Closed
Tracked by #20

Automate C++ include file grouping and ordering using clang-format #1345

harrism opened this issue Feb 21, 2024 · 0 comments · Fixed by #1349
Labels
improvement Improvement / enhancement to an existing function

Comments

@harrism
Copy link
Member

harrism commented Feb 21, 2024

Currently the C++ include files in the RAPIDS codebase are not consistently ordered and grouped.
Inconsistent ordering makes it harder to maintain the codebase, to understand the dependencies,
and to automate changes.

Using clang-format to automate the ordering and grouping of include files will not only add
consistent style, but it will ease automation of changes. For example, We are undertaking a
refactoring of RMM that will replace rmm::mr::device_memory_resource* with
rmm::device_async_resource-ref everywhere in RAPIDS (not just cuDF). This requires adding an
include to MANY files across multiple RAPIDS repos. Getting the location of this include correct
everywhere is very difficult without automatic grouping of headers.

I propose to use clang-format's IncludeCategories settings to automate the ordering and
grouping of include files in the C++ codebase consistently.

Proof-of-concept PRs have been created for RMM and cuDF:

The ordering used for cuDF is given by the following settings.

IncludeBlocks: Regroup
IncludeCategories:
  - Regex:           '^"' # quoted includes
    Priority:        1
  - Regex:           '^<(benchmarks|tests)/' # benchmark includes
    Priority:        2
  - Regex:           '^<cudf_test/' # cuDF includes
    Priority:        3
  - Regex:           '^<cudf/' # cuDF includes
    Priority:        4
  - Regex:           '^<(nvtext|cudf_kafka)' # other libcudf includes
    Priority:        5
  - Regex:           '^<(cugraph|cuml|cuspatial|raft|kvikio)' # Other RAPIDS includes
    Priority:        6
  - Regex:           '^<rmm/' # RMM includes
    Priority:        7
  - Regex:           '^<(thrust|cub|cuda)/' # CCCL includes
    Priority:        8
  - Regex:           '^<(cooperative_groups|cuco|cuda.h|cuda_runtime|device_types|math_constants|nvtx3)' # CUDA includes
    Priority:        8
  - Regex:           '^<.*\..*' # other system includes (e.g. with a '.')
    Priority:        9
  - Regex:           '^<[^.]+' # STL includes (no '.')
    Priority:        10
@harrism harrism added the improvement Improvement / enhancement to an existing function label Feb 21, 2024
@rapids-bot rapids-bot bot closed this as completed in #1349 Mar 6, 2024
rapids-bot bot pushed a commit that referenced this issue Mar 6, 2024
…1349)

This uses the IncludeCategories settings in .clang-format to attempt to enforce our documented #include order in libcuspatial. For discussion, see rapidsai/cudf#15063. This PR uses a subset of the header grouping categories used in that PR.

Note that this also updates the pre-commit configuration to check and correct file copyright years. This comes from rapidsai/cudf#14917.  @KyleFromNVIDIA can you confirm whether or not it's OK to do this? It seems to be working fine.

Closes #1345

Authors:
  - Mark Harris (https://github.com/harrism)

Approvers:
  - Michael Wang (https://github.com/isVoid)

URL: #1349
@github-project-automation github-project-automation bot moved this from Todo to Done in cuSpatial Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

1 participant