Skip to content

fix: allow same-directory header includes - #470

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

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

Conversation

@PNHD

@PNHD PNHD commented Aug 31, 2026 •
edited
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

@coderabbitai

coderabbitai Bot commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: bbf32878-1928-48a8-b999-bf019bc2789d

📥 Commits

Reviewing files that changed from the base of the PR and between 1a8a07f and 1592d0b.

📒 Files selected for processing (5)
  • CHANGELOG.rst
  • cpplint.py
  • cpplint_unittest.py
  • samples/codelite-sample/simple.def
  • samples/vlc-sample/simple.def
🚧 Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.rst

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 permits bare headers found in the source file's directory, including repository-root files. Tests cover root-level and nested paths. Sample reports and the unreleased changelog reflect the corrected diagnostics.

Changes

Include-subdirectory behavior

Layer / File(s) Summary
Resolve same-directory headers
cpplint.py, cpplint_unittest.py		 The check resolves bare headers relative to the source file. It permits existing same-directory headers and reports missing or self-included headers. Tests cover root-level and nested source files.
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 error counts decrease after obsolete include-subdirectory diagnostics are removed.

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

Merge Risk: ⚪ Minimal · up to 1592d

This change narrowly allows valid top-level header includes while preserving existing checks for nested sources and related diagnostics. The supplied tests and checks pass, and no actionable merge-blocking risk remains beyond normal review.

Suggested reviewers: aaronliu0130

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (3 skipped: 3… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes address issue #375 by allowing valid same-directory header includes, including root-level source and header files, while preserving enforcement for missing subdirectory paths.
Out of Scope Changes check ✅ Passed The changelog, regression tests, and sample report updates directly support the include-check fix. No unrelated changes are present.
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. It matches the implementation and regression tests.
Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (3 skipped: 3 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

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 requested 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