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
📝 WalkthroughWalkthroughIWYU now recommends C standard headers for C files and C++ standard headers for C++ files. File extensions and ChangesC header suggestions
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 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
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpplint_unittest.py (1)
1293-1310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover every new file-classification branch.
This regression block exercises only
.cand.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 Agents
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 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.
🤖 Prompt for all review comments with AI agents
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
📒 Files selected for processing (3)
CHANGELOG.rstcpplint.pycpplint_unittest.py
|
|
||
| # 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()).
|
Both implemented in 61df066: C-file status is now stored as state (set in |
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_FILEmissing 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.
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.
|
|
||
| # 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. |
There was a problem hiding this comment.
Why remove this pointer to more information?
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.
| "", | ||
| ) # 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>). |
| # The "C header already included" behaviour is preserved for C files. | ||
| self.TestIncludeWhatYouUse( | ||
| """#include <stdio.h> | ||
| printf("hello world");""", | ||
| "", | ||
| filename="foo.c", | ||
| ) |
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
57db60e to
6858cbe
Compare
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)
5770-5775: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the
foo.ccresult.Line 5775 checks
bar_header_error, which cannot be emitted while lintingtest/foo.cc. The same-basename regression is therefore vacuous. Checkfoo_header_errorinstead.Proposed fix
-assert error_collector.Results().count(bar_header_error) == 0 +assert error_collector.Results().count(foo_header_error) == 0🤖 Prompt for AI Agents
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 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.cpplint.py (1)
5863-5865: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject terminal
.and..path components.
IncludePathUsesDirectoryAliasmissesfoo/.,foo/..,., and..because its patterns require a following/. These are also directory-alias components.Proposed fix
def 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("/"))🤖 Prompt for AI Agents
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.
🤖 Prompt for all review comments with AI agents
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.
🪄 Autofix
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
📒 Files selected for processing (3)
CHANGELOG.rstcpplint.pycpplint_unittest.py
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.rst
| 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[:]📝 Committable suggestion
‼️ 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[:] |
🤖 Prompt for AI Agents
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.
|
Rebased onto develop at 6858cbe (conflict resolved). Both review rounds addressed: |
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.
|
|
||
| * 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) |
|
|
||
| # 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.
| 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?
|
|
||
| # 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>). |
| 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>). |
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.
Fixes #399.
Problem: for C files (
.c/.cu, or any file markedLINT_C_FILE),build/include_what_you_usesuggested 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:
.c/.cu/LINT_C_FILE-marked file usingprintf→ suggests<stdio.h>.cppusingprintf→ still<cstdio>(untranslated).cwith<stdio.h>already included → no suggestion (existing behaviour preserved)std::vector) → unmangledpytest cpplint_unittest.py→ 206 passed; the new regression tests fail without the fix.Summary by CodeRabbit
Bug Fixes
<stdio.h>, instead of C++ equivalents.<cstdio>, for C++ source files.Documentation