added Path::identify()/Path::isHeader2() and deprecated flawed Path::isCPP(), Path::isC() and Path::isHeader()#4681
Conversation
|
An issue with the existing function is that This was relied on in It is quite possible there's a lot of other code which needs to be adjusted but so far the tests did not expose anything else. |
firewave
force-pushed
the
ext
branch
2 times, most recently
from
92b61c6 to
16ac8c1
Compare
what? if the extension is ".cpp" then both isC() and isHeader() returns false right? and isCPP() returns true. |
Yes. I was referring to the code I adjusted in |
firewave
added
the
merge-after-next-release
|
|
||
| try { /* Ticket #8151 */ | ||
| decl = setVarIdParseDeclaration(&tok2, variableMap, scopeStack.top().isExecutable, isCPP(), isC()); | ||
| decl = setVarIdParseDeclaration(&tok2, variableMap, scopeStack.top().isExecutable, isCPP(), isC(), isHeader()); |
There was a problem hiding this comment.
I don't have a good feeling for this isHeader(). you are not supposed to check a header directly:
cppcheck file.h
There was a problem hiding this comment.
People do that and it will happen with IDE integrations. There's also a push to improve handling of static analysis of headers within clang-tidy and CMake.
| continue; | ||
| } | ||
| if (Token::Match(tok2, "struct|union|enum") || (!c && Token::Match(tok2, "class|typename"))) { | ||
| if (Token::Match(tok2, "struct|union|enum") || ((!c || header) && Token::Match(tok2, "class|typename"))) { |
There was a problem hiding this comment.
I think the || header is unfortunate. the question is what language it is.. and header has no information about it.
With this code a header is always assumed to contain C++ code. So if a header with C code contains some int class; variable declaration then after your changes that class variable will be treated as a C++ keyword.
If the user uses --language=c then this || header code will "override" that.
There was a problem hiding this comment.
That whole logic is unfortunate but this keeps the intended behavior intact - I think. At least the tests pass. I will re-visit this when my head is clearer.
There was a problem hiding this comment.
The logic is really strange. Out of the box we treat header files as C and bail out but this code wants to treat those as C++ by default. I think we should get rid of that inconsistency. I will try to perform and write a few more tests for this.
There was a problem hiding this comment.
The changed behavior appears to be consistent and should be applied IMO. I came across this again in #5853 which also exposes the flawed logic.
There was a problem hiding this comment.
The new test results are not yet final. Will take another look later today.
There was a problem hiding this comment.
Okay - I cleaned up that change a lot and updated the test results.
That looks a bit saner now overall but I am a bit baffled why we are able to parse the C++ code in C code. It should bail out and tell you that it is invalid C code.
firewave
force-pushed
the
ext
branch
2 times, most recently
from
c4f5d78 to
8ee166e
Compare
firewave
removed
the
merge-after-next-release
firewave
force-pushed
the
ext
branch
3 times, most recently
from
5e7e5d2 to
44b7e3c
Compare
…::isC()` and `Path::isHeader()`
…` implies `isHeader() == true` / added `TokenList::isHeader()` and `Tokenizer::isHeader()`
There was a problem hiding this comment.
5f24e44: Probably https://trac.cppcheck.net/ticket/12287
d710de3: Probably https://trac.cppcheck.net/ticket/11633
3 participants
None of the
Path::is*()function is working as expected or consistent (detailed via additional unit tests and deprecation comments).I tried to fix them but that would have made made cases with headers quite awkward to implement so I opted to generate a new function. This also optimized
Path::acceptFile()a file.I am not 100% happy with including
settings.hbut I didn't see a place to move this to. If #3774 gets merged we might be able to move theenumintokeywords.h(that could also be renamed).I kept the old function in case somebody else is using these (they are in the API after all) and will remove them after the next release.