fix(iwyu): suggest C headers instead of C++ ones for C files - #456
fix(iwyu): suggest C headers instead of C++ ones for C files#456breidenbach0 wants to merge 2 commits into
Conversation
Sorry, something went wrong.
WalkthroughIWYU now recommends C standard headers for C files and C++ standard headers for C++ files. File extensions and LINT_C_FILE markers determine C-file handling. Tests cover both cases and existing C headers. ChangesC header suggestions
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: aaronliu0130 Sequence Diagram(s)sequenceDiagram
participant SourceFile
participant PerformIncludeWhatYouUse
participant CheckForIncludeWhatYouUse
SourceFile->>PerformIncludeWhatYouUse: provide filename and raw source
PerformIncludeWhatYouUse->>CheckForIncludeWhatYouUse: pass C-file state
CheckForIncludeWhatYouUse->>SourceFile: report C or C++ header suggestion
❌ Failed checks (1 warning)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
There was a problem hiding this comment.
cpplint_unittest.py (1)🤖 Prompt for all review comments with AI agents1293-1310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Cover every new file-classification branch.
This regression block exercises only .c and .cpp; add cases for .cu, LINT_C_FILE, and a non-mapped header to protect the remaining contracts described by the change.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpplint_unittest.py` around lines 1293 - 1310, Extend the include-what-you-use regression tests around PerformIncludeWhatYouUse to cover the remaining file classifications: CUDA files (.cu), files explicitly marked with LINT_C_FILE, and a header extension without a C/C++ mapping. Assert each case’s expected header suggestion or absence of a suggestion, while preserving the existing .c and .cpp assertions.
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Nitpick comments: In `@cpplint_unittest.py`: - Around line 1293-1310: Extend the include-what-you-use regression tests around PerformIncludeWhatYouUse to cover the remaining file classifications: CUDA files (.cu), files explicitly marked with LINT_C_FILE, and a header extension without a C/C++ mapping. Assert each case’s expected header suggestion or absence of a suggestion, while preserving the existing .c and .cpp assertions.
ℹ️ Review info ⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e6160a7-9feb-4764-9b0d-6cdb542dcb6f
📥 CommitsReviewing files that changed from the base of the PR and between bb2f8a6 and a3ff90a.
📒 Files selected for processing (3)- CHANGELOG.rst
- cpplint.py
- cpplint_unittest.py
Sorry, something went wrong.
|
|
||
| # C files should be told to include the C header (e.g. <stdio.h>) rather | ||
| # than its C++ counterpart (e.g. <cstdio>). See #399. | ||
| is_c_file = filename.lower().endswith((".c", ".cu")) or any( |
There was a problem hiding this comment.
I think we should store whether this is a C file as state instead, so we only need to do this exhaustive search once (in ProcessGlobalSuppresions()).
Sorry, something went wrong.
|
Both implemented in 61df066: C-file status is now stored as state (set in ProcessGlobalSuppressions, read in CheckForIncludeWhatYouUse), and the C-file tests go through TestIncludeWhatYouUse with a filename parameter — covering .cu and LINT_C_FILE. |
Sorry, something went wrong.
androvonx95
left a comment
There was a problem hiding this comment.
Rechecked at 57db60e.
Both review notes look addressed: C-file status is stored once via ProcessGlobalSuppressions / ResetNoLintSuppressions, and the new cases go through TestIncludeWhatYouUse(..., filename=), including .cu and LINT_C_FILE.
Verified:
- foo.c / foo.cu / LINT_C_FILE missing include → suggests <stdio.h>
- foo.cpp → still <cstdio>
- already-included <stdio.h> or <cstdio> → no IWYU warning
- non-C-library headers (<vector>, etc.) left untranslated
- full suite: 206 passed
My concerns are resolved — LGTM.
Sorry, something went wrong.
yangfan-yf-yf
left a comment
There was a problem hiding this comment.
Reviewed at 57db60e.
The C-file state is reset before each file and then established from the filename or LINT_C_FILE; only the explicit C-library header mapping is translated. C++ suggestions and non-mapped headers remain unchanged.
Verified locally:
python -m pytest cpplint_unittest.py -k testIncludeWhatYouUse -q --no-cov
2 passed
python -m pytest -q --no-cov
231 passed
git diff --check 57db60e^..57db60e
Passed
LGTM.
Sorry, something went wrong.
|
|
||
| # Whether the file currently being processed is a C file (.c/.cu, or marked | ||
| # with LINT_C_FILE). Set by ProcessGlobalSuppressions and reset by | ||
| # ResetNolintSuppressions, so the determination is done once per file. See #399. |
There was a problem hiding this comment.
| # ResetNolintSuppressions, so the determination is done once per file. See #399. | |
| # ResetNolintSuppressions, so the determination is done once per file. |
Sorry, something went wrong.
There was a problem hiding this comment.
Why remove this pointer to more information?
Sorry, something went wrong.
There was a problem hiding this comment.
The convention of all the commits thus far is we don't add them and instead only add what needs to be commented. There are no such pointers in the current comments, because we haven't actually had a situation where we can't explain something in a few comments yet.
Sorry, something went wrong.
| "", | ||
| ) # Avoid false positives w/ c-style include | ||
| # C files should be told to include the C header (e.g. <stdio.h>) rather | ||
| # than its C++ counterpart (e.g. <cstdio>). See #399. |
There was a problem hiding this comment.
| # than its C++ counterpart (e.g. <cstdio>). See #399. | |
| # than the C++ counterpart (e.g. <cstdio>). |
Sorry, something went wrong.
| # The "C header already included" behaviour is preserved for C files. | ||
| self.TestIncludeWhatYouUse( | ||
| """#include <stdio.h> | ||
| printf("hello world");""", | ||
| "", | ||
| filename="foo.c", | ||
| ) |
There was a problem hiding this comment.
This is already tested
Sorry, something went wrong.
For C files (.c/.cu or LINT_C_FILE), build/include_what_you_use suggested the C++ C-library header (e.g. <cstdio> for printf). It now suggests the C equivalent (<stdio.h>). The C-file determination is factored into _IsCFile (single source, shared by ProcessGlobalSuppressions and the IWYU check) and threaded into CheckForIncludeWhatYouUse as an is_c_file parameter rather than a module global. A whitelist maps the C++ C-library headers to their C counterparts, so non-C-library <c...> headers and C++ files are unaffected. Fixes cpplint#399
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)cpplint_unittest.py (1)cpplint.py (1)5770-5775: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the foo.cc result.
Line 5775 checks bar_header_error, which cannot be emitted while linting test/foo.cc. The same-basename regression is therefore vacuous. Check foo_header_error instead.
Proposed fix🤖 Prompt for AI Agents-assert error_collector.Results().count(bar_header_error) == 0 +assert error_collector.Results().count(foo_header_error) == 0Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpplint_unittest.py` around lines 5770 - 5775, Update the assertion in the multiple-same-basename test around ProcessFileData to count foo_header_error rather than bar_header_error, ensuring the foo.cc lint result is the one being verified while preserving the expected zero count.5863-5865: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject terminal . and .. path components.
IncludePathUsesDirectoryAlias misses foo/., foo/.., ., and .. because its patterns require a following /. These are also directory-alias components.
Proposed fix🤖 Prompt for AI Agentsdef IncludePathUsesDirectoryAlias(include): """Returns whether an include path contains a . or .. component.""" - return include.startswith(("../", "./")) or "/../" in include or "/./" in include + return any(component in {".", ".."} for component in include.split("/"))Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpplint.py` around lines 5863 - 5865, The IncludePathUsesDirectoryAlias function only detects directory-alias components when followed by a slash, missing terminal `.` and `..` components such as `foo/.`, `foo/..`, `.`, and `..`. Add checks to the return statement in IncludePathUsesDirectoryAlias to also detect paths ending with `/.` or `/..` using endswith, and paths that are exactly `.` or `..` using a direct membership test, while preserving the existing middle-component checks.
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Inline comments: In `@cpplint_unittest.py`: - Around line 226-227: Update the setup in the affected test helper so lines is an independent copy of raw_lines rather than an alias. Preserve raw_lines unchanged for _IsCFile() after RemoveMultiLineComments() mutates lines, ensuring LINT_C_FILE markers inside multi-line comments are detected. --- Outside diff comments: In `@cpplint_unittest.py`: - Around line 5770-5775: Update the assertion in the multiple-same-basename test around ProcessFileData to count foo_header_error rather than bar_header_error, ensuring the foo.cc lint result is the one being verified while preserving the expected zero count. In `@cpplint.py`: - Around line 5863-5865: The IncludePathUsesDirectoryAlias function only detects directory-alias components when followed by a slash, missing terminal `.` and `..` components such as `foo/.`, `foo/..`, `.`, and `..`. Add checks to the return statement in IncludePathUsesDirectoryAlias to also detect paths ending with `/.` or `/..` using endswith, and paths that are exactly `.` or `..` using a direct membership test, while preserving the existing middle-component checks.
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info ⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f259127-e0b0-4810-9681-47ee7375b133
📥 CommitsReviewing files that changed from the base of the PR and between 57db60e and 7914485.
📒 Files selected for processing (3)- CHANGELOG.rst
- cpplint.py
- cpplint_unittest.py
- CHANGELOG.rst
Sorry, something went wrong.
| raw_lines = code.split("\n") | ||
| lines = raw_lines |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep raw_lines independent from lines.
lines = raw_lines aliases the same list. RemoveMultiLineComments() then mutates raw_lines before _IsCFile() reads it. This helper cannot reproduce detection of a LINT_C_FILE marker in a multi-line comment.
Proposed fix raw_lines = code.split("\n")
-lines = raw_lines
+lines = raw_lines[:]‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| raw_lines = code.split("\n") | |
| lines = raw_lines | |
| raw_lines = code.split("\n") | |
| lines = raw_lines[:] |
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpplint_unittest.py` around lines 226 - 227, Update the setup in the affected test helper so lines is an independent copy of raw_lines rather than an alias. Preserve raw_lines unchanged for _IsCFile() after RemoveMultiLineComments() mutates lines, ensuring LINT_C_FILE markers inside multi-line comments are detected.
Sorry, something went wrong.
|
Rebased onto develop at 6858cbe (conflict resolved). Both review rounds addressed: |
Sorry, something went wrong.
aaronliu0130
left a comment
There was a problem hiding this comment.
Please read the contributing guide's passage on rebasing/force pushes.
Some earlier comments remain unaddressed.
Sorry, something went wrong.
|
|
||
| * Fixed a whitespace/newline false positive for control conditions containing lambdas. (#410) | ||
| * We now error on relative include paths (``./``, ``../``). (#432) | ||
| * For C files, build/include_what_you_use now suggests the C header (e.g. ``<stdio.h>``) instead of its C++ counterpart (e.g. ``<cstdio>``). (#399) |
Sorry, something went wrong.
|
|
||
| # Second, look for missing includes. | ||
| cpplint.CheckForIncludeWhatYouUse(filename, lines, include_state, error_collector, io) | ||
| is_c_file = cpplint._IsCFile(filename, raw_lines) |
There was a problem hiding this comment.
This means we still run this twice for each file, which was exactly what we were trying to avoid with the move to state.
Sorry, something went wrong.
| include_state = cpplint._IncludeState() | ||
| nesting_state = cpplint.NestingState() | ||
| lines = code.split("\n") | ||
| raw_lines = code.split("\n") |
There was a problem hiding this comment.
What is the point of making a new variable here?
Sorry, something went wrong.
|
|
||
| # Maps the C++ C-library headers to their C equivalents so that, for C files, | ||
| # build/include_what_you_use suggests the C header (e.g. <stdio.h> rather than | ||
| # <cstdio>). See https://github.com/cpplint/cpplint/issues/399. |
There was a problem hiding this comment.
| # <cstdio>). See https://github.com/cpplint/cpplint/issues/399. | |
| # <cstdio>). |
Sorry, something went wrong.
| header_stripped[0] == "c" and (header_stripped[1:] + ".h") in include_dict | ||
| ): | ||
| # For C files, suggest the C header (e.g. <stdio.h>) rather than | ||
| # the C++ one (e.g. <cstdio>). See #399. |
There was a problem hiding this comment.
| # the C++ one (e.g. <cstdio>). See #399. | |
| # the C++ one (e.g. <cstdio>). |
Sorry, something went wrong.
yangfan-yf-yf
left a comment
There was a problem hiding this comment.
The current head has one additional merge blocker in its required checks: Ruff reports FBT002 at cpplint.py:7162 because is_c_file=False is a positional boolean default. Both current call sites already pass is_c_file by keyword, so making it keyword-only (for example, io=codecs, *, is_c_file=False) should preserve behavior and clear the check.
The focused include-what-you-use tests and the full suite pass on this head (2 passed; 231 passed). Please resolve the Ruff failure along with the existing maintainer threads before re-requesting review.
Sorry, something went wrong.
PNHD
left a comment
There was a problem hiding this comment.
Re-reviewed current head 7914485.
The functional IWYU behavior is covered and the focused/full tests pass, but the required Ruff check still reports FBT002 for the positional boolean default is_c_file=False.
That blocker and the existing maintainer threads are already documented, so I won't duplicate them inline. Requesting changes until the required checks are green and those threads are resolved.
Sorry, something went wrong.
Reviewers
Requested changes must be addressed to merge this pull request.
Fixes #399.
Problem: for C files (.c/.cu, or any file marked LINT_C_FILE), build/include_what_you_use suggested the C++ C-library header — e.g. Add #include <cstdio> for printf — even though C code should include <stdio.h>.
Cause: the suggestion text always named the mapped header (<cstdio>). The check already recognised that an included <stdio.h> satisfies <cstdio>, but when it did fire, it still named the C++ header regardless of file type.
Fix: translate known C++ C-library headers (<cXXX>) to their C equivalents (<XXX.h>) when the file is a C file. The mapping is whitelisted (<cstdio>→<stdio.h>, <cstdint>→<stdint.h>, …) so non-C-library <c…> headers are left untouched, and C++ files are unaffected.
Testing:
Summary by CodeRabbit
Bug Fixes
Documentation