build: enable clang-tidy misc-include-cleaner for include hygiene#820
build: enable clang-tidy misc-include-cleaner for include hygiene#820yadavay-amzn wants to merge 1 commit into
Conversation
|
Not too sure, but should we pursue LLVM include-cleaner [1]? [1] https://clang.llvm.org/extra/clang-tidy/checks/misc/include-cleaner.html |
|
Good suggestion - pivoted to misc-include-cleaner. Since the project already runs clang-tidy 22 incrementally via cpp-linter, this enforces include hygiene on changed lines with no new tooling, so I dropped the separate IWYU workflow, iwyu.imp mapping, and CMake option. Added misc-include-cleaner.IgnoreHeaders for the vendored third-party headers (arrow, nanoarrow, nlohmann, spdlog, CRoaring, cpr) to suppress noise. The diff is now just a few lines in .clang-tidy. Updated the PR title/description accordingly. |
wgtmac
left a comment
There was a problem hiding this comment.
Thanks for improving this! I've left a minor comment. LMKWYT.
| - key: misc-include-cleaner.IgnoreHeaders | ||
| value: 'arrow/.*;nanoarrow/.*;nlohmann/.*;spdlog/.*;roaring/.*;cpr/.*' |
There was a problem hiding this comment.
| - key: misc-include-cleaner.IgnoreHeaders | |
| value: 'arrow/.*;nanoarrow/.*;nlohmann/.*;spdlog/.*;roaring/.*;cpr/.*' | |
| - key: misc-include-cleaner.IgnoreHeaders | |
| value: 'arrow/.*;avro/.*;aws/.*;cpr/.*;gmock/.*;gtest/.*;nanoarrow/.*;nlohmann/.*;parquet/.*;roaring/.*;spdlog/.*;sqlpp23/.*;sqlite3\.h;thrift/.*;utf8proc\.h;zlib\.h' |
IgnoreHeaders should cover the other third-party headers currently included from src/iceberg and tests too. In particular, utf8proc.h was added on main after this PR branched, and existing dependencies like Avro/Parquet, sqlpp23, zlib, sqlite3, gtest/gmock, and optional AWS/Thrift headers can otherwise produce include-cleaner noise.
What
Closes #73. Enables clang-tidy's
misc-include-cleanercheck to enforce include hygiene.How
Adds
misc-include-cleanerto.clang-tidy. Because the project already runs clang-tidy 22 incrementally (changed-lines-only) via the cpp-linter workflow, include hygiene is now enforced on new/changed code with no additional tooling, workflow, or mapping file.IgnoreHeadersis configured for third-party headers (arrow, nanoarrow, nlohmann-json, spdlog, CRoaring, cpr) to suppress noise from vendored includes.Notes
This supersedes the original IWYU-based approach (a separate workflow +
iwyu.impmapping + CMake option), per the review discussion with @zhjwpku:misc-include-cleanerintegrates with the existing clang-tidy setup and is far lower-maintenance for the same goal.This contribution was authored with assistance from Claude Opus 4.8, in line with the Iceberg guidelines for AI-assisted contributions (https://iceberg.apache.org/contribute/#guidelines-for-ai-assisted-contributions). All changes were reviewed and tested by the author.