CppCheck shows lint messages from included files as well as the current
file. Filter out those, since they don't make much sense in the context
of arc lint.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Commits
- rARCaabbdbd2ab95: Filter out messages from included files
Before this patch, arc lint using ArcanistCppcheckLinter. Note that lint messages
from included files appear pointing to a line in the active file.
After this patch, only messages from the active file are included.
Diff Detail
Diff Detail
- Branch
- filter-cppcheck-messages
- Lint
Lint Passed - Unit
Tests Passed
Event Timeline
Comment Actions
(See inline.)
| src/lint/linter/ArcanistCppcheckLinter.php | ||
|---|---|---|
| 118 | Can we setPath($file) (or some approximation) here instead? Arcanist should be able to handle this correctly in the general case, and route/display the message properly, although obviously we're passing the wrong data right now. $file might require canonicalization along these lines: $root = $this->getEngine()->getWorkingCopy()->getProjectRoot(); $local_file = Filesystem::readablePath($file, $root); // Maybe test if $local_file Filesystem::isDescendant() of $root, too? // Not sure how far-reaching the messages are. If there are reasons this isn't practical, the filtering fix seems reasonable to me, but this fix seems a little nicer if it does work. | |
| src/lint/linter/ArcanistCppcheckLinter.php | ||
|---|---|---|
| 118 | I'm not familiar (yet) with the Arcanist source, but as far as I can see the simple filtering is what's needed. CppCheck now generates lints like this:
Where all the messages refer to other files than the one being linter. It seems to me that we don't need to do anything else than just filter them out. | |
Comment Actions
Ah, okay -- these warnings aren't caused by changes to src/ArticleType.cpp, they just happen to be included in it? Elsewhere, we have some warnings like:
src/Thing.c:22 Function definition x() is broken... src/Thing.h:19 Function declaration for broken x() is in header here...
In this case, raising the extra warning is desirable, because it's directly related to "Thing.c". It looks like these are just incidental, though. I agree that we should just filter them, in that case.
Comment Actions
Oh, actually, one more thing -- by default, resolvePath() uses the CWD. I expect this patch won't work if you run it like this:
cd project/ cd blah/blah/blah/ # cd into somewhere deep in the project arc lint ../../../src/ArticleType.cpp
Can you make sure that works? If it doesn't, $this->getEngine()->getWorkingCopy()->getProjectRoot(); should get the project root, and Filesystem::resolvePath($file, $root) should be comparable to $path or $this->getActivePath() (which should have the same value).
Comment Actions
Yes, it works this way. Since the linter is called with $this->getEngine()->getFilePathOnDisk($path) which returns a full path, the value of $file will also always be a full path. I changed the patch to use $path instead of $this->getActivePath() to be more concise.