Repository navigation
Conversation
| } | ||
| } | ||
|
|
||
| std::vector<std::string> PathMatch::unmatched() const { |
There was a problem hiding this comment.
This is an AI review take it with a grain of salt. Please feel free to reject it by clicking on Resolve button
Header excludes can never be "used", because Path::acceptFile() filters out headers before ignored.match() is called. So -iinc/h.h src or -i*.h src now prints:
cppcheck: filename exclusion does not apply to header (.h and .hpp) files.
cppcheck: Please use --suppress for ignoring results from the header files.
cppcheck: error: unused ignore/exclude path 'inc/h.h'. To hide warnings in certain files use suppressions instead.
and exits with code 1. Before, this was only an informational message. If failing is intended for headers, the first two lines could probably be merged into the error, or dropped, so the same thing isn't said twice. If it isn't intended, header patterns could be left out of the unused check.
582af1e to
7f4334b
Compare
|
@claude review |
| // paths inside a matched directory are not traversed, so a pattern that is | ||
| // covered by a matched pattern is considered used | ||
| const Filemode mode = !s.empty() && PathIterator::issep(s.back(), mSyntax) ? Filemode::directory : Filemode::regular; | ||
| return std::none_of(mMatchedPatterns.cbegin(), mMatchedPatterns.cend(), [&](const std::string& matched) { |
There was a problem hiding this comment.
False negative: this "covered by a matched pattern" check runs against every matched pattern, but the reasoning only holds for patterns that matched a directory. Only directory matches stop the traversal. A file pattern never hides anything.
Examples where a stale pattern goes unreported:
-ifoo.cpp -inonexistent/foo.cpp:foo.cppmatcheslib/foo.cpp, andmatch("foo.cpp", "nonexistent/foo.cpp")is true, so the second pattern counts as used.-i*_test.cpp -imissing_test.cpp: the glob matches some file, somissing_test.cppis never reported.
Suggestion: keep track of which patterns matched with Filemode::directory, and only use those for this check.
|
|
||
| const auto& unmatched = matcher.unmatched(); | ||
| if (!unmatched.empty()) { | ||
| mLogger.printError("unused ignore/exclude path '" + unmatched.front() + "'. To hide warnings in certain files use suppressions instead."); |
There was a problem hiding this comment.
A couple of points about making this a hard error:
- Only
unmatched.front()is reported. If a user has several stale-ipaths, they have to fix one and re-run for each. Listing all of them would be easier to use (same for the project branch below). -ifoo.hnow always fails, because headers are never passed tomatch(). The user first sees the existing "filename exclusion does not apply to header files" info message and then this error. It may be worth making the two messages consistent.- Builds that share one ignore list across different path arguments will now fail. So will builds that ignore generated directories that don't exist yet, e.g.
-ibuild/on a fresh checkout. Is a hard error (rather than a warning) intended here? If so, the release note should call it out as a breaking change.
|
Overall the approach looks good:
🤖 Generated with Claude Code |
No description provided.