Skip to content

valueflow.cpp: avoid some copies related to ErrorPath #4160

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
firewave merged 1 commit into from
Jul 20, 2022
+9 −11
Merged

valueflow.cpp: avoid some copies related to ErrorPath #4160

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 9 additions & 11 deletions lib/valueflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2485,24 +2485,23 @@ struct ValueFlowAnalyzer : Analyzer {
tok->astParent()->astOperand2()->getKnownValue(ValueFlow::Value::ValueType::INT);
assert(rhsValue);
if (evalAssignment(*value, getAssign(tok->astParent(), d), *rhsValue)) {
const std::string info("Compound assignment '" + tok->astParent()->str() + "', assigned value is " +
value->infoString());
std::string info("Compound assignment '" + tok->astParent()->str() + "', assigned value is " +
value->infoString());
if (tok->astParent()->str() == "=")
value->errorPath.clear();
value->errorPath.emplace_back(tok, info);
value->errorPath.emplace_back(tok, std::move(info));
} else {
assert(false && "Writable value cannot be evaluated");
// TODO: Don't set to zero
value->intvalue = 0;
}
} else if (tok->astParent()->tokType() == Token::eIncDecOp) {
bool inc = tok->astParent()->str() == "++";
std::string opName(inc ? "incremented" : "decremented");
const std::string opName(inc ? "incremented" : "decremented");
if (d == Direction::Reverse)
inc = !inc;
value->intvalue += (inc ? 1 : -1);
const std::string info(tok->str() + " is " + opName + "', new value is " + value->infoString());
value->errorPath.emplace_back(tok, info);
value->errorPath.emplace_back(tok, tok->str() + " is " + opName + "', new value is " + value->infoString());
}
}

Expand Down Expand Up @@ -3852,6 +3851,7 @@ struct LifetimeStore {
const Token *tok2 = v.tokvalue;
ErrorPath er = v.errorPath;
const Variable *var = getLifetimeVariable(tok2, er);
// TODO: the inserted data is never used
er.insert(er.end(), errorPath.begin(), errorPath.end());
Copy link
Copy Markdown
Collaborator Author

@firewave firewave Jun 1, 2022

Choose a reason for hiding this comment

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

@pfultz2
Please take a look. Seems like this is inserted the wrong way.

Would be great if we could detect this but I guess it's a variant of https://trac.cppcheck.net/ticket/4593.

Copy link
Copy Markdown
Collaborator Author

@firewave firewave Jul 5, 2022

Choose a reason for hiding this comment

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

@pfultz2 Please take a look.

Feel free to merge as well.

if (!var)
continue;
Expand Down Expand Up @@ -4399,7 +4399,7 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger
else if (c == LifetimeCapture::ByValue)
value.lifetimeScope = ValueFlow::Value::LifetimeScope::ThisValue;
value.tokvalue = tok2;
value.errorPath.push_back({tok2, "Lambda captures the 'this' variable here."});
value.errorPath.emplace_back(tok2, "Lambda captures the 'this' variable here.");
Copy link
Copy Markdown
Collaborator Author

@firewave firewave Jun 1, 2022

Choose a reason for hiding this comment

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

I would have expected for clang-tidy to detect this but maybe that check is still in review.

Copy link
Copy Markdown
Collaborator Author

@firewave firewave Jun 3, 2022

Choose a reason for hiding this comment

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

Turns out we have modernize-use-emplace disabled in our .clang-tidy config.

The detection of unnecessary temporary object was to clang-tidy just today: llvm/llvm-project@987f9cb

value.lifetimeKind = ValueFlow::Value::LifetimeKind::Lambda;
capturedThis = true;
// Don't add the value a second time
Expand Down Expand Up @@ -6808,8 +6808,7 @@ static void valueFlowSwitchVariable(TokenList *tokenlist, SymbolDatabase* symbol
std::list<ValueFlow::Value> values;
values.emplace_back(MathLib::toLongNumber(tok->next()->str()));
values.back().condition = tok;
const std::string info("case " + tok->next()->str() + ": " + vartok->str() + " is " + tok->next()->str() + " here.");
values.back().errorPath.emplace_back(tok, info);
values.back().errorPath.emplace_back(tok, "case " + tok->next()->str() + ": " + vartok->str() + " is " + tok->next()->str() + " here.");
bool known = false;
if ((Token::simpleMatch(tok->previous(), "{") || Token::simpleMatch(tok->tokAt(-2), "break ;")) && !Token::Match(tok->tokAt(3), ";| case"))
known = true;
Expand All @@ -6820,8 +6819,7 @@ static void valueFlowSwitchVariable(TokenList *tokenlist, SymbolDatabase* symbol
tok = tok->next();
values.emplace_back(MathLib::toLongNumber(tok->next()->str()));
values.back().condition = tok;
const std::string info2("case " + tok->next()->str() + ": " + vartok->str() + " is " + tok->next()->str() + " here.");
values.back().errorPath.emplace_back(tok, info2);
values.back().errorPath.emplace_back(tok, "case " + tok->next()->str() + ": " + vartok->str() + " is " + tok->next()->str() + " here.");
}
for (std::list<ValueFlow::Value>::const_iterator val = values.begin(); val != values.end(); ++val) {
valueFlowReverse(tokenlist,
Expand Down