Skip to content

Add test cases for assign operators with default implementation#2767

Merged
danmar merged 2 commits into
cppcheck-opensource:mainfrom
dan-42:fix_eq_operator_default
Sep 3, 2020
Merged

Add test cases for assign operators with default implementation#2767
danmar merged 2 commits into
cppcheck-opensource:mainfrom
dan-42:fix_eq_operator_default

Conversation

@dan-42
Copy link
Copy Markdown
Contributor

@dan-42 dan-42 commented Sep 1, 2020

👋 @danmar

Sorry to bother you again 🙃

So I have found another issue, and after cutting down my code this fails

struct foobar
{
  auto operator=(const foobar&) -> foobar&;
};
auto foobar::operator=(const foobar&) -> foobar& = default;

with:

$ cppcheck --enable=all "foobar.cpp"
Checking foobar.cpp ...
foobar.cpp:0:0: error: Internal Error: Invalid syntax [cppcheckError]
^
foobar.cpp:0:0: note: Internal Error: Invalid syntax
^
foobar.cpp:0:0: note: Internal Error: Invalid syntax
^
  • adding { } instead of default works
  • adding default inside the struct definition works
  • changing it to the classical form foobar& foobar::operator=(const foobar&) = default; works

So I added two test cases to reproduce the issue.
cppcheck failes here:

void CheckClass::initializeVarList(const Function &func, std::list<const Function *> &callstack, const Scope *scope, std::vector<Usage> &usage)
{
    if (!func.functionScope)
        throw InternalError(nullptr, "Internal Error: Invalid syntax"); // #5702
    bool initList = func.isConstructor();

call comes from this function CheckClass::constructors()
and on line 147 it is checked:

   for (const Function &func : scope->functionList) {
            if (!func.hasBody() || !(func.isConstructor() || func.type == Function::eOperatorEqual))
                continue;

So I wonder why the check for cpp func.type == Function::eOperatorEqual

So the issue boils down that func.functionScope is nullptr

Could you point me in a direction, is this more of a tokenizing issue?

Comment thread test/testclass.cpp Outdated
@dan-42
Copy link
Copy Markdown
Contributor Author

dan-42 commented Sep 1, 2020

deleted, because this was the wrong direction for this investigation

@dan-42
Copy link
Copy Markdown
Contributor Author

dan-42 commented Sep 1, 2020

@danmar I added my new findings see above

@dan-42
Copy link
Copy Markdown
Contributor Author

dan-42 commented Sep 1, 2020

Ok maybe not I just saw the xml attribute originalName="->" 😞

@danmar
Copy link
Copy Markdown
Collaborator

danmar commented Sep 1, 2020

Yes the token is ".".. well that is a very old decision that made more sense before we had the AST. I think we can consider using "->" someday again but if somebody wants to change it then it must be done carefully.

@dan-42 dan-42 force-pushed the fix_eq_operator_default branch from d504ef7 to bc993df Compare September 1, 2020 12:45
@dan-42
Copy link
Copy Markdown
Contributor Author

dan-42 commented Sep 1, 2020

@danmar So I found the issue and indeed it was in the symboldatabase
and the "default" was not detected when using trailing return types.

Comment thread lib/symboldatabase.cpp Outdated
Token::Match(closeParen, ") . %type% = default ;") ||
Token::Match(closeParen, ") . %type% & = default ;") ||
Token::Match(closeParen, ") . const %type% & = default ;") ||
Token::Match(closeParen, ") . const %type% = default ;") ||
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danmar I'm pretty sure this can be done in a better way right?
and it also does not cover all cases yet.
So I'm hoping I will get some help here, please.

Copy link
Copy Markdown
Collaborator

@danmar danmar Sep 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well a start could be:

Token::Match(closeParen, ") . const| %type% &| = default ;"))

but not sure if that is the ultimate solution, maybe we have some code to handle trailing return type and stuff..

Copy link
Copy Markdown
Collaborator

@danmar danmar Sep 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is some copy/paste problems in the SymbolDatabase. Maybe we should have some utility function to parse function attributes . There is a loop in Function::Function below a comment // parse function attributes that looks promising. I am spontanously wondering if that code can be broken out and reused somehow.

But.. the Token::Match pattern I provided could be a quick fix I could merge that as a start.. if you add a proper test in TestSymbolDatabase also. And then anybody can look at the refactoring later in a separate PR.

Copy link
Copy Markdown
Collaborator

@danmar danmar Sep 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another way is to reuse Tokenizer::isFunctionHead.. something like:

const Token *endtok = mTokenizer->isFunctionHead(closeParent->link(), ";");
if (endtok && Token::simpleMatch(endtok->tokAt(-2), "= default ;"))
.....

.. well we have some options. I believe a small fix to start with, with a proper test and a Token::Match is a good first step.

As far as I see, isFunctionHead does not handle trailing return types but it's very important that it can handle those.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, thank you I will clean it up and add a proper test for this.
maybe I'll find the time tomorrow evening

@dan-42 dan-42 force-pushed the fix_eq_operator_default branch from bc993df to 716694c Compare September 2, 2020 19:30
@dan-42
Copy link
Copy Markdown
Contributor Author

dan-42 commented Sep 2, 2020

@danmar So here you go.
I cleaned it up and added a testcase for the symboldatabase.
Hope this is what you had in mind?

@danmar danmar merged commit 974b6fb into cppcheck-opensource:main Sep 3, 2020
@danmar
Copy link
Copy Markdown
Collaborator

danmar commented Sep 3, 2020

Have you been added to our AUTHORS file? If not, you deserve to have your name there. What name would you like to add there?

@dan-42
Copy link
Copy Markdown
Contributor Author

dan-42 commented Sep 3, 2020

@danmar Wow thank you for the merge and also to give me the honors to call my self a "cppcheck author" :-)
I'm OK with my full name "Daniel Friedrich"

chrchr-github added a commit to chrchr-github/cppcheck that referenced this pull request Dec 4, 2023
chrchr-github added a commit that referenced this pull request Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants