Visitar URL original
fix: allow same-directory header includes by PNHD · Pull Request #470 · cpplint/cpplint · GitHub
Skip to content

fix: allow same-directory header includes - #470

Open
PNHD wants to merge 4 commits into
cpplint:developfrom
PNHD:fix/375-top-level-include
Open

fix: allow same-directory header includes#470
PNHD wants to merge 4 commits into
cpplint:developfrom
PNHD:fix/375-top-level-include

Conversation

@PNHD

@PNHD PNHD commented Aug 31, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Fixes #375.

Summary

  • resolve bare quoted headers against the directory of the file being linted before emitting build/include_subdir
  • allow a bare include when a different header file actually exists alongside the source, including files under repository subdirectories
  • keep build/include_subdir enforcement when the bare header does not resolve locally, and keep self-includes subject to the existing warning
  • add regression coverage for both repository-root and nested same-directory headers, plus nested missing-header cases
  • update affected sample expectations and the unreleased changelog

Testing

  • regression test first reproduced the src/main.cpp + src/utils.hpp false positive from False positive for top-level header #375
  • pytest --no-cov cpplint_unittest.py -k testBuildInclude: passed after the fix
  • full test suite: passed
  • pylint cpplint.py: passed
  • repository pre-commit checks: passed

Summary by CodeRabbit

  • Bug Fixes

    • Improved include-subdirectory checks for files accessed through symlinks, hard links, and equivalent paths.
    • Prevented false warnings when source files include headers from the same directory without a directory prefix.
    • Corrected self-include detection across equivalent file paths.
    • Updated sample lint reports to reflect the reduced false-positive warning count.

  • Documentation

    • Clarified changelog wording for same-directory header includes.

@coderabbitai

coderabbitai Bot commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: e2fed310-149d-4547-b34d-58b9f853b9c0

📥 Commits

Reviewing files that changed from the base of the PR and between 508206a and d160830.

📒 Files selected for processing (1)
  • cpplint_unittest.py

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📝 Walkthrough

Walkthrough

The build/include_subdir check now resolves bare headers against the source file’s physical directory and preserves warnings for missing or self-included headers. Tests cover path aliases and invalid paths. Sample reports and the changelog reflect the updated behavior.

Changes

Include-subdirectory behavior

Layer / File(s) Summary
Resolve same-directory headers
cpplint.py, cpplint_unittest.py		 The check uses real paths and file identity checks for same-directory headers, symlink paths, symlink aliases, hardlink aliases, and invalid header paths.
Update documented and sample results
CHANGELOG.rst, samples/codelite-sample/simple.def, samples/vlc-sample/simple.def		 The changelog describes same-directory header includes. Sample reports remove obsolete diagnostics and reduce their error counts.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: walidbi200

Merge Risk: ⚪ Minimal · up to d1608

No concrete correctness or merge-blocking risk remains for this focused change.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: allowing same-directory header includes.
Linked Issues check ✅ Passed Issue #375 requires build/include_subdir to allow a quoted header when that header exists in the source file directory. The PR resolves the source path, checks the bare header in that directory, and…
Out of Scope Changes check ✅ Passed The code change, regression tests, sample expectation updates, and changelog update support the same-directory include fix in issue #375. No unrelated change is demonstrated.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

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

@PNHD PNHD changed the title fix: allow top-level header includes fix: allow same-directory header includes Aug 31, 2026
@PNHD PNHD mentioned this pull request Aug 31, 2026
Open
walidbi200
walidbi200 suggested changes Sep 13, 2026
View reviewed changes

@walidbi200 walidbi200 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed current head 1592d0b. The normal #375 case is fixed and the tests are green, but I found two path-handling cases that seem important to fix before merging.

One is a source path that goes through a symlink and then ... In that case cpplint can open one real file, while FileInfo.FullName() normalizes the path to a different lexical directory. The new same-directory header check can then look beside the wrong path. I reproduced both outcomes: it can warn even when the real sibling header exists, and it can suppress the warning when only an unrelated header exists in the normalized directory.

The other case is self-includes through filesystem aliases. If self.h includes alias.h and alias.h is a symlink or hard link to self.h, develop still reports build/include_subdir, but this PR suppresses it. os.path.samefile() confirms the two paths refer to the same file. The current normpath() / normcase() comparison only compares path strings, so it doesn't catch that identity case.

I also checked the ordinary same-directory behavior, missing headers, the sample-output changes, and the full test suite; those parts look good. I think these two filesystem cases should get regression tests and a fix before merge.

walidbi200
walidbi200 approved these changes Sep 20, 2026
View reviewed changes

@walidbi200 walidbi200 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Re-reviewed current head d160830. Both findings from my previous review are fixed: I reproduced the symlink+.. false positive/false negative and both alias suppressions on 1592d0b, and all four behave correctly on this head.

The realpath/samefile change is narrowly scoped, the new regression tests fail without the corresponding fixes, and I also checked normal same-directory includes, self-includes, missing and invalid paths, symlinked sources, broken symlinks, and the fallback path handling without finding a new correctness issue.

Focused and full suites pass locally (236 tests, 96.48% coverage), along with pylint, mypy, pre-commit, and git diff --check.

No remaining blocker from me on this head. LGTM.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@walidbi200 walidbi200 walidbi200 approved these changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

False positive for top-level header

2 participants

@PNHD @walidbi200