Skip to content

Enforce east const via clang-format QualifierAlignment - #22834

Merged
rapids-bot[bot] merged 8 commits into
NVIDIA:mainfrom
PointKernel:enforce-east-const-clang-format
Jul 1, 2026
Merged

Enforce east const via clang-format QualifierAlignment#22834
rapids-bot[bot] merged 8 commits into
NVIDIA:mainfrom
PointKernel:enforce-east-const-clang-format

Conversation

@PointKernel

@PointKernel PointKernel commented Jun 10, 2026

Copy link
Copy Markdown
Member

Description

This PR updates the clang-format configuration to consistently enforce east const formatting across the codebase.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@PointKernel
PointKernel requested review from a team as code owners June 10, 2026 02:04
@PointKernel
PointKernel requested review from bdice and mattgara June 10, 2026 02:04
@PointKernel PointKernel added the non-breaking Non-breaking change label Jun 10, 2026
@github-actions github-actions Bot added libcudf Affects libcudf (C++/CUDA) code. Java Affects Java cuDF API. labels Jun 10, 2026
@PointKernel PointKernel added improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. Java Affects Java cuDF API. and removed libcudf Affects libcudf (C++/CUDA) code. Java Affects Java cuDF API. labels Jun 10, 2026
@bdice

bdice commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Historically we did not enable this for some discovered reason, but I do not recall the details.

https://github.com/rapidsai/cudf/blob/2a52096f88e3cafb4c6f089c6ca801ff8c9aa810/cpp/doxygen/developer_guide/DEVELOPER_GUIDE.md?plain=1#L160-L163

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: caeeb46e-4b71-48d7-849b-a762990f853a

📥 Commits

Reviewing files that changed from the base of the PR and between f0bcb42 and 967b44b.

📒 Files selected for processing (69)
  • cpp/benchmarks/common/ndsh_data_generator/ndsh_data_generator.cpp
  • cpp/benchmarks/common/ndsh_data_generator/random_column_generator.cu
  • cpp/benchmarks/common/ndsh_data_generator/random_column_generator.hpp
  • cpp/benchmarks/common/nvbench_utilities.cpp
  • cpp/benchmarks/groupby/group_nunique.cpp
  • cpp/benchmarks/io/cuio_common.cpp
  • cpp/benchmarks/io/parquet/parquet_reader_filter.cpp
  • cpp/examples/hybrid_scan_io/hybrid_scan_multifile_single_step.cpp
  • cpp/examples/hybrid_scan_io/hybrid_scan_multifile_two_step.cpp
  • cpp/include/cudf/strings/detail/gather.cuh
  • cpp/include/cudf_test/nanoarrow_utils.hpp
  • cpp/src/io/comp/brotli_dict.cpp
  • cpp/src/io/comp/compression.cpp
  • cpp/src/io/comp/debrotli.cu
  • cpp/src/io/comp/decompression.cpp
  • cpp/src/io/comp/gpuinflate.cu
  • cpp/src/io/comp/snap.cu
  • cpp/src/io/comp/unsnap.cu
  • cpp/src/io/json/json_column.cu
  • cpp/src/io/json/process_tokens.cu
  • cpp/src/io/orc/stripe_data.cu
  • cpp/src/io/orc/stripe_enc.cu
  • cpp/src/io/orc/writer_impl.cu
  • cpp/src/io/parquet/decode_fixed.cu
  • cpp/src/io/parquet/page_decode.cuh
  • cpp/src/io/parquet/page_string_decode.cu
  • cpp/src/io/utilities/base64_utilities.cpp
  • cpp/src/merge/merge.cu
  • cpp/src/partitioning/partitioning.cu
  • cpp/src/reductions/scan/ewm.cu
  • cpp/src/rolling/jit/kernel.cu
  • cpp/src/strings/copying/copy_range.cu
  • cpp/src/utilities/cuda_memcpy.cu
  • cpp/src/utilities/stream_pool.cpp
  • cpp/src/utilities/time_utils.cuh
  • cpp/tests/copying/copy_range_tests.cpp
  • cpp/tests/copying/scatter_list_scalar_tests.cpp
  • cpp/tests/copying/scatter_tests.cpp
  • cpp/tests/dictionary/scatter_test.cpp
  • cpp/tests/groupby/shift_tests.cpp
  • cpp/tests/interop/from_arrow_device_test.cpp
  • cpp/tests/interop/from_arrow_host_test.cpp
  • cpp/tests/interop/from_arrow_test.cpp
  • cpp/tests/interop/to_arrow_device_test.cpp
  • cpp/tests/interop/to_arrow_host_test.cpp
  • cpp/tests/io/csv_test.cpp
  • cpp/tests/io/fst/fst_test.cu
  • cpp/tests/io/json/json_test.cpp
  • cpp/tests/io/json/json_tree.cpp
  • cpp/tests/io/orc_test.cpp
  • cpp/tests/io/parquet_common.cpp
  • cpp/tests/io/parquet_misc_test.cpp
  • cpp/tests/io/parquet_v2_test.cpp
  • cpp/tests/iterator/optional_iterator_test_numeric.cu
  • cpp/tests/iterator/pair_iterator_test_numeric.cu
  • cpp/tests/iterator/value_iterator_test_transform.cu
  • cpp/tests/join/join_tests.cpp
  • cpp/tests/join/mixed_join_tests.cu
  • cpp/tests/labeling/label_bins_tests.cpp
  • cpp/tests/merge/merge_string_test.cpp
  • cpp/tests/merge/merge_test.cpp
  • cpp/tests/replace/replace_tests.cpp
  • cpp/tests/rolling/grouped_rolling_test.cpp
  • cpp/tests/rolling/rolling_test.cpp
  • cpp/tests/round/round_tests.cpp
  • cpp/tests/streams/copying_test.cpp
  • cpp/tests/strings/array_tests.cpp
  • cpp/tests/utilities/identify_stream_usage.cpp
  • cpp/tests/wrappers/timestamps_test.cu
✅ Files skipped from review due to trivial changes (44)
  • cpp/tests/strings/array_tests.cpp
  • cpp/tests/iterator/pair_iterator_test_numeric.cu
  • cpp/benchmarks/io/parquet/parquet_reader_filter.cpp
  • cpp/examples/hybrid_scan_io/hybrid_scan_multifile_two_step.cpp
  • cpp/src/io/utilities/base64_utilities.cpp
  • cpp/tests/io/fst/fst_test.cu
  • cpp/src/io/json/json_column.cu
  • cpp/examples/hybrid_scan_io/hybrid_scan_multifile_single_step.cpp
  • cpp/src/strings/copying/copy_range.cu
  • cpp/src/io/parquet/decode_fixed.cu
  • cpp/src/reductions/scan/ewm.cu
  • cpp/tests/labeling/label_bins_tests.cpp
  • cpp/tests/interop/from_arrow_device_test.cpp
  • cpp/src/partitioning/partitioning.cu
  • cpp/src/utilities/stream_pool.cpp
  • cpp/src/utilities/cuda_memcpy.cu
  • cpp/tests/interop/from_arrow_test.cpp
  • cpp/tests/replace/replace_tests.cpp
  • cpp/benchmarks/io/cuio_common.cpp
  • cpp/tests/iterator/value_iterator_test_transform.cu
  • cpp/tests/join/join_tests.cpp
  • cpp/src/io/json/process_tokens.cu
  • cpp/tests/io/parquet_v2_test.cpp
  • cpp/tests/copying/copy_range_tests.cpp
  • cpp/src/io/orc/writer_impl.cu
  • cpp/tests/round/round_tests.cpp
  • cpp/tests/io/json/json_tree.cpp
  • cpp/src/io/orc/stripe_data.cu
  • cpp/tests/interop/from_arrow_host_test.cpp
  • cpp/tests/io/parquet_misc_test.cpp
  • cpp/tests/join/mixed_join_tests.cu
  • cpp/tests/io/parquet_common.cpp
  • cpp/benchmarks/groupby/group_nunique.cpp
  • cpp/src/io/comp/decompression.cpp
  • cpp/tests/iterator/optional_iterator_test_numeric.cu
  • cpp/tests/io/csv_test.cpp
  • cpp/include/cudf/strings/detail/gather.cuh
  • cpp/tests/merge/merge_string_test.cpp
  • cpp/tests/interop/to_arrow_device_test.cpp
  • cpp/tests/merge/merge_test.cpp
  • cpp/tests/interop/to_arrow_host_test.cpp
  • cpp/tests/rolling/rolling_test.cpp
  • cpp/benchmarks/common/nvbench_utilities.cpp
  • cpp/tests/io/json/json_test.cpp
🚧 Files skipped from review as they are similar to previous changes (24)
  • cpp/tests/copying/scatter_list_scalar_tests.cpp
  • cpp/src/io/parquet/page_decode.cuh
  • cpp/src/utilities/time_utils.cuh
  • cpp/include/cudf_test/nanoarrow_utils.hpp
  • cpp/src/merge/merge.cu
  • cpp/benchmarks/common/ndsh_data_generator/random_column_generator.hpp
  • cpp/tests/io/orc_test.cpp
  • cpp/src/io/comp/compression.cpp
  • cpp/src/io/comp/snap.cu
  • cpp/src/io/parquet/page_string_decode.cu
  • cpp/src/io/orc/stripe_enc.cu
  • cpp/src/rolling/jit/kernel.cu
  • cpp/src/io/comp/debrotli.cu
  • cpp/tests/wrappers/timestamps_test.cu
  • cpp/tests/streams/copying_test.cpp
  • cpp/benchmarks/common/ndsh_data_generator/random_column_generator.cu
  • cpp/benchmarks/common/ndsh_data_generator/ndsh_data_generator.cpp
  • cpp/src/io/comp/unsnap.cu
  • cpp/tests/utilities/identify_stream_usage.cpp
  • cpp/tests/groupby/shift_tests.cpp
  • cpp/tests/dictionary/scatter_test.cpp
  • cpp/src/io/comp/gpuinflate.cu
  • cpp/tests/copying/scatter_tests.cpp
  • cpp/tests/rolling/grouped_rolling_test.cpp

📝 Walkthrough

Summary by CodeRabbit

  • Style
    • Standardized const/qualifier formatting across C++, CUDA, Java, and test code for consistency.
    • Updated clang-format behavior to align C++ type qualifiers in a uniform way.
  • Tests
    • Adjusted benchmark and unit-test code to match the updated const/qualifier conventions.
  • Chores
    • Refreshed SPDX/copyright header text across many source files.

Walkthrough

This PR adds QualifierAlignment: Right to .clang-format and applies equivalent const/volatile/CUDA qualifier-order reformatting across benchmark, core C++, test, and Java JNI native files. No functional logic, control flow, or public APIs are changed; a few host_span<const char* const> signatures are updated to host_span<char const* const>.

Changes

QualifierAlignment Right sweep

Layer / File(s) Summary
Formatting policy switch
.clang-format
Adds QualifierAlignment: Right to clang-format configuration.
Benchmarks and streaming surface updates
cpp/benchmarks/..., cpp/examples/hybrid_scan_io/..., cpp/libcudf_streaming/benchmarks/..., cpp/libcudf_streaming/include/.../device_bloom_filter.hpp
Reorders const/qualifier placement in NDS-H data generator vocab spans, nvbench utilities, cuio benchmarks, parquet reader filter, hybrid scan examples, partition/shuffle benchmarks, and the bloom filter view return type.
Core C++ runtime updates
cpp/librtcx/embed.hpp, cpp/src/hash/*, cpp/src/io/comp/*, cpp/src/io/orc/*, cpp/src/io/json/*, cpp/src/io/parquet/*, cpp/src/interop/to_arrow_host.cu, cpp/src/merge/merge.cu, cpp/src/partitioning/partitioning.cu, cpp/src/reductions/scan/ewm.cu, cpp/src/rolling/jit/kernel.cu, cpp/src/strings/copying/copy_range.cu, cpp/src/utilities/*, cpp/include/cudf/strings/detail/gather.cuh
Reorders const/volatile/__constant__ qualifiers in hashing constant tables, compression/decompression casts, ORC/Parquet/JSON decode paths, merge/partition/rolling kernel parameters, and utility helpers, plus a host_span element-type fix.
C++ test suite normalization
cpp/tests/**, cpp/libcudf_streaming/tests/**, cpp/include/cudf_test/nanoarrow_utils.hpp
Reorders const qualifiers on local variables, loop variables, scalar reference_wrapper types, exception catch clauses, and lambda parameters across copying, dictionary, groupby, interop, IO, iterator, join, labeling, merge, reductions, replace, rolling, streams, and utility tests.
Java JNI native formatting updates
java/src/main/native/src/AggregationJni.cpp, java/src/main/native/src/TableJni.cpp
Reorders const qualifier placement on local wrapper variables in JNI aggregation and table sort/order/merge entry points.

Estimated code review effort: 3 (Moderate) | ~30 minutes

Suggested labels: libcudf, Java, improvement, non-breaking, CMake

Suggested reviewers: bdice, davidwendt, vyasr, mhaseeb123, lamarrr

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 32.35% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately captures the main change: enforcing east const via clang-format qualifier alignment.
Description check ✅ Passed The description is directly related to the changeset and matches the clang-format east const update.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ast-grep (0.44.0)
cpp/benchmarks/common/ndsh_data_generator/ndsh_data_generator.cpp

ast-grep exited with code unknown

cpp/benchmarks/common/ndsh_data_generator/random_column_generator.hpp

ast-grep exited with code unknown

cpp/benchmarks/common/nvbench_utilities.cpp

ast-grep exited with code unknown

  • 17 others

Comment @coderabbitai help to get the list of available commands.

@ttnghia

ttnghia commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Historically we did not enable this for some discovered reason, but I do not recall the details.

https://github.com/rapidsai/cudf/blob/2a52096f88e3cafb4c6f089c6ca801ff8c9aa810/cpp/doxygen/developer_guide/DEVELOPER_GUIDE.md?plain=1#L160-L163

Probably the new version is better now. We should enforce that and workaround if there issue with it.

@PointKernel

Copy link
Copy Markdown
Member Author

The last similar attempt was quite early, e.g. #13493 where the bulk east-const conversion via clang-format's QualifierAlignment ran into issues on clang 14.

The upstream caveat is still there as of clang-format 20 but I reviewed the generated diff, and on our codebase it comes out clean: Pointer and volatile qualifiers reorder correctly because only the pointee qualifier moves (e.g. const char* const to char const* const, volatile int32_t* to int32_t volatile*), preserving the underlying types with no semantic swaps such as const T* to T* const. So I think we're safe to enforce it. If some future edge case ever trips it, // clang-format off/on coudl be the local escape hatch.

@vyasr

vyasr commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

IIRC @karthikeyann ended up using multiple tools to generate his old PRs, not just clang-format, because all of them tripped up in different ways. If everything is clean now that's great.

@karthikeyann

Copy link
Copy Markdown
Contributor

clang-format missed a lot of places in previous attempt. Even after fixing with it, other tools found more instances.
clang-formats lack of complete semantic information.
https://releases.llvm.org/14.0.0/tools/clang/docs/ClangFormatStyleOptions.html#:~:text=COULD%20lead%20to%20incorrect%20code%20formatting

Warning
Setting QualifierAlignment to something other than Leave, COULD lead to incorrect code formatting due to incorrect decisions made due to clang-formats lack of complete semantic information. As such extra care should be taken to review code changes made by the use of this option.

…clang-format

# Conflicts:
#	cpp/libcudf_streaming/benchmarks/bench_shuffle.cpp
#	cpp/libcudf_streaming/include/cudf_streaming/detail/device_bloom_filter.hpp
#	cpp/libcudf_streaming/tests/streaming/test_shuffler.cpp
#	cpp/libcudf_streaming/tests/test_shuffler.cpp
@PointKernel

Copy link
Copy Markdown
Member Author

It looks like the only cases clang-format missed were instances where const was used together with __constant__. We should really be using constexpr instead of const there, and once that's changed, the issue goes away. Fixed in f0bcb42

@vyasr vyasr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the changes look correct (as expected, it is also changing other qualifiers like volatile, which is fine). Let's merge it and see how it holds up for a while.

@PointKernel

Copy link
Copy Markdown
Member Author

/merge

@rapids-bot
rapids-bot Bot merged commit 76318f8 into NVIDIA:main Jul 1, 2026
136 checks passed
@PointKernel
PointKernel deleted the enforce-east-const-clang-format branch July 1, 2026 23:03
@karthikeyann

karthikeyann commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Nice work 🎉
Please consider using other tools like https://github.com/biasmv/westerly etc and self created tools to do another run on entire repo. if these tools return nothing, then we can pretty much depend on clang-format.

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 Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants