Skip to content

some cleanups#4308

Merged
danmar merged 3 commits intocppcheck-opensource:mainfrom
firewave:cleanups-2
Jul 26, 2022
Merged

some cleanups#4308
danmar merged 3 commits intocppcheck-opensource:mainfrom
firewave:cleanups-2

Conversation

@firewave
Copy link
Copy Markdown
Collaborator

@firewave firewave commented Jul 25, 2022

These were encountered by looking at the changes suggested by the upcoming misc-const-correctness clang-tidy check and reviewing similar code. More PRs are coming.

@firewave
Copy link
Copy Markdown
Collaborator Author

Some of these things should have been caught by us. Will dig into those soon and file tickets.

@firewave
Copy link
Copy Markdown
Collaborator Author

firewave commented Jul 25, 2022

Just for fun - scanning mame_regtest with --enable=all --inconclusive:

Clang 13 (DISABLE_VALUEFLOW=1) 2,458,148,600 -> 2,458,152,166
Clang 13 52,576,874,852 -> 52,575,491,628

Not so much fun. Quite baffling how it takes more if we removed code. Will try to look into it. The flakiness of the compiler optimizations are getting extremely annoying.

Turns out the regression is caused by std::make_pair(). In one case I see it having an average Ir of 247 compared with the previous 207. There's less memcpy() calls but everything else looks identical. So the stuff I removed was already being removed by the DCE/DSE it seems.

@firewave firewave marked this pull request as draft July 25, 2022 23:40
@firewave
Copy link
Copy Markdown
Collaborator Author

Will the std::make_pair() changes reverted it actually shows a slight improvement as expected: 2,458,142,936.

@firewave firewave marked this pull request as ready for review July 25, 2022 23:52
@danmar danmar merged commit 0005be1 into cppcheck-opensource:main Jul 26, 2022
@firewave firewave deleted the cleanups-2 branch July 26, 2022 06:43
Comment thread lib/valueflow.cpp
auto captureVariable = [&](const Token* tok2, LifetimeCapture c, std::function<bool(const Token*)> pred) {
if (varids.count(tok->varId()) > 0)
return;
ErrorPath errorPath;
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I assume this is https://trac.cppcheck.net/ticket/9823 since the unused variable is inside a lamdba.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Turns out there's a different issue related to having lamdba variables: https://trac.cppcheck.net/ticket/11215

Comment thread lib/valueflow.cpp
return conds;

Condition cond;
ValueFlow::Value true_value;
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

These are not detected since the class has another constructor with unknown implementation: https://trac.cppcheck.net/ticket/11216

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That other constructor has no side effects either but that is not visible. It might be helpful to configure this via a library: https://trac.cppcheck.net/ticket/11217

Comment thread lib/valueflow.cpp
if (ftok && argn >= 0) {
if (ftok->function()) {
std::vector<ValueType> result;
std::vector<const Variable*> argsVars = getArgumentVars(ftok, argn);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This was a bitch to reduce - super weird: https://trac.cppcheck.net/ticket/11218

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.

2 participants