added `Path::identify()`/`Path::isHeader2()` and deprecated flawed `Path::isCPP()`, `Path::isC()` and `Path::isHeader()` #4681

firewave · 2023-01-03T23:57:51Z

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.h but I didn't see a place to move this to. If #3774 gets merged we might be able to move the enum into keywords.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.

firewave · 2023-01-04T00:37:06Z

An issue with the existing function is that isC() == false implies isHeader() == true where as isCPP() == true is the other way around and implies isHeader() == true (and even missing an extension).

This was relied on in tokenize.cpp by setVarIdParseDeclaration().

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.

danmar · 2023-01-07T22:40:19Z

An issue with the existing function is that isC() == false implies isHeader() == true where as isCPP() == true is the other way around and implies isHeader() == true (and even missing an extension).

what? if the extension is ".cpp" then both isC() and isHeader() returns false right? and isCPP() returns true.

firewave · 2023-01-07T22:50:31Z

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 setVarIdParseDeclaration(). That has handling for headers by checking for isC() == false which meant that everything but a C source file is matched - see 88990ba. Sorry for not linking that before.

lib/path.h

danmar · 2023-01-30T07:33:12Z

lib/tokenize.cpp

@@ -4053,7 +4053,7 @@ void Tokenizer::setVarIdPass1()
            }

            try { /* Ticket #8151 */
-                decl = setVarIdParseDeclaration(&tok2, variableMap, scopeStack.top().isExecutable, isCPP(), isC());
+                decl = setVarIdParseDeclaration(&tok2, variableMap, scopeStack.top().isExecutable, isCPP(), isC(), isHeader());


I don't have a good feeling for this isHeader(). you are not supposed to check a header directly:

cppcheck file.h

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.

danmar · 2023-01-30T08:10:25Z

lib/tokenize.cpp

@@ -3563,7 +3563,7 @@ static bool setVarIdParseDeclaration(const Token **tok, const VariableMap& varia
                tok2 = tok2->linkAt(1)->next();
                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"))) {


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.

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.

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.

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.

The new test results are not yet final. Will take another look later today.

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.

…::isC()` and `Path::isHeader()`

…` implies `isHeader() == true` / added `TokenList::isHeader()` and `Tokenizer::isHeader()`

chrchr-github

5f24e44: Probably https://trac.cppcheck.net/ticket/12287
d710de3: Probably https://trac.cppcheck.net/ticket/11633

danmar

lgtm

firewave force-pushed the ext branch 2 times, most recently from 92b61c6 to 16ac8c1 Compare January 4, 2023 12:17

firewave marked this pull request as ready for review January 5, 2023 12:50

firewave added the merge-after-next-release Wait with merging this PR until after the next Release label Jan 18, 2023

firewave force-pushed the ext branch from 16ac8c1 to 4dc4a6d Compare January 29, 2023 18:56

danmar reviewed Jan 30, 2023

View reviewed changes

firewave force-pushed the ext branch from 4dc4a6d to ed4a6b4 Compare January 30, 2023 15:38

firewave marked this pull request as draft January 30, 2023 15:52

firewave mentioned this pull request Apr 13, 2023

CppCheck: removed changing of debugwarnings for non-accepted files #4965

Merged

firewave force-pushed the ext branch 2 times, most recently from c4f5d78 to 8ee166e Compare August 14, 2023 11:00

This was referenced Jan 6, 2024

make sure the language for a Token/TokenList is always determined #5853

Merged

moved Language from settings.h to standards.h #5854

Merged

firewave removed the merge-after-next-release Wait with merging this PR until after the next Release label Jan 6, 2024

firewave force-pushed the ext branch 3 times, most recently from 5e7e5d2 to 44b7e3c Compare January 8, 2024 08:49

firewave marked this pull request as ready for review January 8, 2024 08:49

firewave force-pushed the ext branch from 44b7e3c to 4837a65 Compare January 8, 2024 09:31

firewave added 6 commits January 8, 2024 12:02

added Path::identify() and deprecated flawed Path::isCPP(), `Path…

caceec6

…::isC()` and `Path::isHeader()`

added Path::isHeader2() as a convenience function

f87b356

fixed assumption in setVarIdParseDeclaration() that `isC() == false…

461decf

…` implies `isHeader() == true` / added `TokenList::isHeader()` and `Tokenizer::isHeader()`

path.cpp: suppress knownConditionTrueFalse false positive

d710de3

path.cpp: mitigated unusedFunction selfcheck warning

f64f81f

path.cpp: mitigated uninitvar selfcheck false positive

5f24e44

firewave force-pushed the ext branch from 4837a65 to 5f24e44 Compare January 8, 2024 11:02

chrchr-github reviewed Jan 8, 2024

View reviewed changes

danmar approved these changes Jan 13, 2024

View reviewed changes

firewave merged commit d5a054c into danmar:main Jan 13, 2024
64 checks passed

firewave deleted the ext branch January 13, 2024 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added `Path::identify()`/`Path::isHeader2()` and deprecated flawed `Path::isCPP()`, `Path::isC()` and `Path::isHeader()` #4681

added `Path::identify()`/`Path::isHeader2()` and deprecated flawed `Path::isCPP()`, `Path::isC()` and `Path::isHeader()` #4681

firewave commented Jan 3, 2023

firewave commented Jan 4, 2023

danmar commented Jan 7, 2023

firewave commented Jan 7, 2023 •

edited

Loading

danmar Jan 30, 2023

firewave Jan 30, 2023 •

edited

Loading

danmar Jan 30, 2023

firewave Jan 30, 2023

firewave Aug 14, 2023

firewave Jan 6, 2024

firewave Jan 8, 2024

firewave Jan 8, 2024

chrchr-github left a comment •

edited

Loading

danmar left a comment

added Path::identify()/Path::isHeader2() and deprecated flawed Path::isCPP(), Path::isC() and Path::isHeader() #4681

added Path::identify()/Path::isHeader2() and deprecated flawed Path::isCPP(), Path::isC() and Path::isHeader() #4681

Conversation

firewave commented Jan 3, 2023

firewave commented Jan 4, 2023

danmar commented Jan 7, 2023

firewave commented Jan 7, 2023 • edited Loading

danmar Jan 30, 2023

Choose a reason for hiding this comment

firewave Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

danmar Jan 30, 2023

Choose a reason for hiding this comment

firewave Jan 30, 2023

Choose a reason for hiding this comment

firewave Aug 14, 2023

Choose a reason for hiding this comment

firewave Jan 6, 2024

Choose a reason for hiding this comment

firewave Jan 8, 2024

Choose a reason for hiding this comment

firewave Jan 8, 2024

Choose a reason for hiding this comment

chrchr-github left a comment • edited Loading

Choose a reason for hiding this comment

danmar left a comment

Choose a reason for hiding this comment

added `Path::identify()`/`Path::isHeader2()` and deprecated flawed `Path::isCPP()`, `Path::isC()` and `Path::isHeader()` #4681

added `Path::identify()`/`Path::isHeader2()` and deprecated flawed `Path::isCPP()`, `Path::isC()` and `Path::isHeader()` #4681

firewave commented Jan 7, 2023 •

edited

Loading

firewave Jan 30, 2023 •

edited

Loading

chrchr-github left a comment •

edited

Loading