Repository navigation
Fix #15126 FP syntaxError (function try block with override in struct) - #8955
chrchr-github wants to merge 8 commits into
Conversation
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
The fix works for the common forms, void f() [const|noexcept|noexcept(true)] override|final try {...} catch(...) {...}, and I found no regressions. It only handles a single override or final directly after ) or after a keyword, though. Two forms with override still give the same syntaxError:
- a ref-qualifier:
void i() & override try,void i() && override try,void i() const & override try - a trailing return type:
auto k() -> void override try
The base version fails on these forms without override as well (void i() & try, auto k() -> void try), so they are a gap that already existed rather than a regression. It is still a gap in the same ticket area. When final or override is used as a variable name before a try inside a function body, the PR gives no new false positive.
Testing
I built cppcheck and testrunner in pr/. ./testrunner TestTokenizer TestGarbage TestSymbolDatabase TestSimplifyTokens TestClass passed with 0 failures.
Handled correctly, no errors:
const overridenoexcept overridenoexcept(true) overrideoverride finalint final = 0; try {...}inside a function bodyint override[2]; ... try {...}inside a function body
Still syntaxError, ./cppcheck -q x.cpp in each case:
struct E : B {
void i() & override try {} catch (...) {}
};error: syntax error: keyword 'try' is not allowed in class/struct/union/enum scope [syntaxError]
These forms give the same error:
void i() && override try {} catch (...) {}void i() const & override try {} catch (...) {}auto k() -> void override try {} catch (...) {}
Without override, the base version also rejects void i() & try {} catch (...) {} and auto k() -> void try {} catch (...) {}, so this limitation existed before the PR.
Automated review of aa91b3a by Claude. The pull request was built and tested locally.
| continue; | ||
| const Token* par = tok->previous(); | ||
| while (par && par->isKeyword()) | ||
| while (par && (par->isKeyword() || Token::Match(par, "final|override"))) |
There was a problem hiding this comment.
AI review
This loop still misses override or final after a ref-qualifier or a trailing return type. These still produce syntaxError:
struct E : B { void i() & override try {} catch (...) {} };
struct F : B { auto k() -> void override try {} catch (...) {} };&/&& and -> type before try already fail in base without override, so this is a gap that existed before, not a regression. It may still be worth covering here, or noting in the ticket.
|
Test results for commit aa91b3a (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8955/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
The fix now covers ref-qualifiers and trailing return types, but the trailing return type part only works when the return type is made of plain names and ::. The backward walk in simplifyFunctionTryCatch stops at any other token, so these still give syntaxError:
- a pointer return type:
auto k() -> int* override try {} catch (...) {} - a template return type:
auto k() -> std::vector<int> override try {} catch (...) {}
Forms that do work now include -> std::size_t final, -> const int& override, -> decltype(1) override and -> int (*)() override, so whether this case works depends on the spelling of the return type. Apart from this, I found no new problems: I saw no regressions in function bodies or in constructor try blocks. The earlier review already covered -> T try without override, which still fails in both base and the PR.
Testing
I built cppcheck and testrunner in pr/, and cppcheck in base/. ./testrunner TestTokenizer TestGarbage TestSymbolDatabase TestSimplifyTokens TestClass TestFunctions TestVarID passed: 1627 tests, 0 failures.
Still syntaxError. I put each member below in struct B{virtual ~B();}; struct D : B { <member> }; and ran ./cppcheck -q t.cpp:
auto k() -> int* override try {} catch (...) {}
auto k() -> std::vector<int> override try {} catch (...) {}Output:
t1.cpp:3:28: error: syntax error: keyword 'try' is not allowed in class/struct/union/enum scope [syntaxError]
t2.cpp:3:40: error: syntax error: keyword 'try' is not allowed in class/struct/union/enum scope [syntaxError]
Fixed by the PR (base gives syntaxError, the PR gives none):
-> const int& override-> decltype(1) override-> int (*)() override-> std::size_t finalconst & noexcept overridenoexcept(false) overridethrow() overridevoid f() &override final
Some of these give missingReturn, which is correct for those bodies.
Function bodies. I compared the --debug-normal output of base and the PR on try blocks after if (x) MACRO, while (x), x = x && p; and for (;;), and on a constructor try block with an initializer list. The output is identical.
Automated review of bb851af by Claude. The pull request was built and tested locally.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found one new problem: the backward walk in simplifyFunctionTryCatch stops at the first ) it finds. If that ) is nested inside a template argument of a trailing return type, it is not the end of the parameter list, so isFunctionHead() fails. For example, auto f() -> W<decltype(1)> override try {...} catch (...) {...} still gives syntaxError. -> decltype(1) override try works only because there the ) that is found happens to be followed directly by override. Base also rejects this form, so it is a remaining gap rather than a regression. Apart from that, I found no new problems. The walk can only find more function try blocks than base does, not fewer: base stopped at the same ) or failed.
Testing
I built cppcheck and testrunner in pr/. ./testrunner TestTokenizer TestGarbage TestSymbolDatabase passed with 0 failures.
Still syntaxError:
template<class T> struct W {};
struct B { virtual W<int> f(); };
struct D : B {
auto f() -> W<decltype(1)> override try { return {}; } catch (...) { return {}; }
};$ ./cppcheck -q g.cpp
g.cpp:4:41: error: syntax error: keyword 'try' is not allowed in class/struct/union/enum scope [syntaxError]
auto f() -> std::vector<decltype(x)> override try ... gives the same error.
No errors for any of these:
-> decltype(1) override trynoexcept(sizeof(int) > 2) override trynoexcept(n(1) > 0) override trynoexcept(noexcept(B()) && true) override trynoexcept(sizeof(int) > 2) try, withoutoverride
Automated review of c10d38f by Claude. The pull request was built and tested locally.
| continue; | ||
| const Token* par = tok->previous(); | ||
| while (par && par->isKeyword()) | ||
| while (par && !Token::Match(par, "[);]")) |
There was a problem hiding this comment.
AI review
The walk stops at the first ). When that ) is inside a template argument of the trailing return type, isFunctionHead() sees ) > override try and returns null. So this still gives syntaxError:
struct D : B {
auto f() -> W<decltype(1)> override try { return {}; } catch (...) { return {}; }
};A fix could skip linked tokens while walking back: when par is a > or ) with a link, jump to par->link(). It could also keep walking until par->link()->previous() is a name that is not decltype/noexcept/throw. Either way, the walk would reach the ) that closes the parameter list.
|
Test results for commit c10d38f (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8955/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
danmar
left a comment
danmar
left a comment
There was a problem hiding this comment.
spontanously the AI review comment looks correct to me.
However if you want to limit the scope of this PR and ignore that comment then feel free to do it. A ticket wouldn't hurt then so we don't forget it.
|
merge conflicts.. |
ad10769 to
d3c04bf
Compare
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found one new problem. When a trailing return type ends in an array declarator, a function try block with override still gives syntaxError. Examples are auto k() -> int(*)[3] override try ... and -> int(&)[3] override try .... The backward walk stops at the ) of (*)/(&). isFunctionHead() then takes its ) [ branch, skips [3], and rejects override. Base fails on this form too, so it is a gap that already existed, not a regression.
The cases from the earlier reviews now work: ref-qualifiers, -> int*, -> W<int> and -> W<decltype(1)> with override. -> T try without override still fails, as C1 already noted. I found no false positives and no changes in function bodies. A merge conflict was mentioned in C11, and I could not check whether it has been resolved.
Earlier comments
9 earlier comments, 2 open
| # | Comment | By | Where | Status |
|---|---|---|---|---|
| 1 | override after a ref-qualifier or a trailing return type still gives syntaxError |
AI review | general | ✅ Fixed |
| 2 | -> T try / & try without override fail (pre-existing) |
AI review | general | |
| 3 | Walk misses override/final after ref-qualifier or trailing return type | AI review | lib/tokenize.cpp:9430 |
✅ Fixed |
| 4 | -> int* override try and -> std::vector<int> override try give syntaxError |
AI review | general | ✅ Fixed |
| 5 | Walk stops at * or <...> in the trailing return type |
AI review | lib/tokenize.cpp:9430 |
✅ Fixed |
| 6 | -> W<decltype(1)> override try gives syntaxError |
AI review | general | ✅ Fixed |
| 7 | Walk stops at a ) nested in a template argument |
AI review | lib/tokenize.cpp:9430 |
✅ Fixed |
| 8 | Maintainer agrees with the AI review on nested ) in template args |
@danmar | general | ✅ Fixed |
| 9 | Merge conflicts need resolving | @danmar | general |
@chrchr-github, please fix, resolve or reply to the open comments: #2, #9.
For #2 (false negative), it is enough to create a Trac ticket and link it here. It does not need to be fixed in this PR.
Testing
I built cppcheck and testrunner in pr/ and cppcheck in base/.
./testrunner TestTokenizer TestGarbage TestSymbolDatabase TestSimplifyTokens TestClass TestVarID: 1572 tests, 0 failed.
Each member below was put in template<class T> struct W {}; struct B { virtual ~B(); }; struct D : B { <member> }; and checked with cppcheck -q:
| member | PR | base |
|---|---|---|
auto k() -> int* override try {} catch (...) {} |
missingReturn (correct) | syntaxError |
auto k() -> W<int> override try ... |
missingReturn | syntaxError |
auto k() -> W<decltype(1)> override try ... |
missingReturn | syntaxError |
auto k() -> W<(1>0)> override try ... |
missingReturn | syntaxError |
void k() & try ... |
none | syntaxError |
void k() override [[]] try ... |
none | syntaxError |
auto k() -> int(*)() override try ... |
none | syntaxError |
auto k() -> void try ... (no override, already reported in C1) |
syntaxError | syntaxError |
auto k() -> int(*)[3] override try ... |
syntaxError | syntaxError |
auto k() const -> int(&)[3] override try ... |
syntaxError | syntaxError |
Reproducer:
template<class T> struct W {};
struct B { virtual ~B(); };
struct D : B {
auto k() -> int(*)[3] override try { return nullptr; } catch (...) { return nullptr; }
};$ ./cppcheck -q k.cpp
k.cpp:4:36: error: syntax error: keyword 'try' is not allowed in class/struct/union/enum scope [syntaxError]
Function bodies: I compared the --debug-normal output of base and the PR on try blocks after else, case 1:, inside a lambda with a trailing return type, after do, after a label, after for (...) and after if (x > y). I also tested a requires (sizeof(T) > 1) function try block. The output is identical and there are no warnings.
Automated review of d3c04bf by Claude. The pull request was built and tested locally.
| continue; | ||
| const Token* par = tok->previous(); | ||
| while (par && par->isKeyword()) | ||
| while (par && !Token::Match(par, "[);]")) { |
There was a problem hiding this comment.
AI review
When the trailing return type ends in an array declarator, the walk stops at the ) of (*)/(&), which is not the end of the parameter list. isFunctionHead() then takes its ) ;|{|[ branch, skips [3] and finds override instead of try, so this still gives syntaxError (base does too):
struct D : B {
auto k() -> int(*)[3] override try { return nullptr; } catch (...) { return nullptr; }
};k.cpp:4:36: error: syntax error: keyword 'try' is not allowed in class/struct/union/enum scope [syntaxError]
-> int(&)[3] override try fails the same way. -> int(*)() override works only because the ) that the walk finds happens to be followed directly by override.
No description provided.