Skip to content
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

Jump to bottom

ValueFlow: pass ErrorLogger by reference into ValueFlow::setValues() / removed need for LifetimeStore::Context #5299

Merged
merged 9 commits into from
Apr 4, 2024
+179 −144
Merged

ValueFlow: pass ErrorLogger by reference into ValueFlow::setValues() / removed need for LifetimeStore::Context #5299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d6e12b8
ValueFlow: pass `ErrorLogger` by reference into `ValueFlow::setValues()`
firewave Mar 7, 2023
4595196
avoid some unchecked pointer dereferences
firewave Aug 7, 2023
e27d171
testrunner: adjusted several `Tokenizer` creations
firewave Aug 7, 2023
eef052c
valueflow.cpp: removed need for `LifetimeStore::Context`
firewave Mar 9, 2024
76a459a
Tokenizer: made `ErrorLogger` parameter mandatory in constructor
firewave Mar 11, 2024
c8de69b
importproject.cpp: use `TokenList` instead of `Tokenizer` in `ItemDef…
firewave Mar 11, 2024
92cbc6c
tokenize.cpp: mitigated `unusedFunction` selfcheck warning
firewave Mar 11, 2024
127d361
valueflow.cpp: mitigated `naming-privateMemberVariable` selfcheck war…
firewave Mar 11, 2024
589d6f3
valueflow.cpp: removed `mutable` from `lifetimeStore::forwardTok`
firewave Apr 2, 2024
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
2 changes: 1 addition & 1 deletion lib/cppcheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@ unsigned int CppCheck::checkClang(const std::string &path)
clangimport::parseClangAstDump(tokenizer, ast);
ValueFlow::setValues(tokenizer.list,
const_cast<SymbolDatabase&>(*tokenizer.getSymbolDatabase()),
this,
*this,
mSettings,
&s_timerResults);
if (mSettings.debugnormal)
Expand Down
16 changes: 7 additions & 9 deletions lib/forwardanalyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,12 @@ namespace {
struct ForwardTraversal {
enum class Progress { Continue, Break, Skip };
enum class Terminate { None, Bail, Inconclusive };
ForwardTraversal(const ValuePtr<Analyzer>& analyzer, const TokenList& tokenList, ErrorLogger* const errorLogger, const Settings& settings)
ForwardTraversal(const ValuePtr<Analyzer>& analyzer, const TokenList& tokenList, ErrorLogger& errorLogger, const Settings& settings)
: analyzer(analyzer), tokenList(tokenList), errorLogger(errorLogger), settings(settings)
{}
ValuePtr<Analyzer> analyzer;
const TokenList& tokenList;
ErrorLogger* const errorLogger;
ErrorLogger& errorLogger;
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes the class not assignable. This should be a pointer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That ship has already sailed - some one line above.

I will take a look at adding that clang-tidy check about this (there was a case of false positives which I am not sure is fixed yet) and there's also #4785 and some discussions which related to that (which apparently are not linked).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI There also would be a compiler error if we would actually try to assign those types. I ran into this issue while working on it. So it is not a silent failure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also I encountered the first issue we have with this in some other changes. I will try to figure out how to properly handle it so it can be easily be applied in the future. Nothing to do here though since this doesn't change anything as we were using references before.

const Settings& settings;
Analyzer::Action actions;
bool analyzeOnly{};
Expand Down Expand Up @@ -839,11 +839,9 @@ namespace {
}

void reportError(Severity severity, const std::string& id, const std::string& msg) {
if (errorLogger) {
ErrorMessage::FileLocation loc(tokenList.getSourceFilePath(), 0, 0);
const ErrorMessage errmsg({std::move(loc)}, tokenList.getSourceFilePath(), severity, msg, id, Certainty::normal);
errorLogger->reportErr(errmsg);
}
ErrorMessage::FileLocation loc(tokenList.getSourceFilePath(), 0, 0);
const ErrorMessage errmsg({std::move(loc)}, tokenList.getSourceFilePath(), severity, msg, id, Certainty::normal);
errorLogger.reportErr(errmsg);
}

static bool isFunctionCall(const Token* tok)
Expand Down Expand Up @@ -904,7 +902,7 @@ namespace {
};
}

Analyzer::Result valueFlowGenericForward(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const TokenList& tokenList, ErrorLogger* const errorLogger, const Settings& settings)
Analyzer::Result valueFlowGenericForward(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const TokenList& tokenList, ErrorLogger& errorLogger, const Settings& settings)
{
if (a->invalid())
return Analyzer::Result{Analyzer::Action::None, Analyzer::Terminate::Bail};
Expand All @@ -915,7 +913,7 @@ Analyzer::Result valueFlowGenericForward(Token* start, const Token* end, const V
return Analyzer::Result{ ft.actions, ft.terminate };
}

Analyzer::Result valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const TokenList& tokenList, ErrorLogger* const errorLogger, const Settings& settings)
Analyzer::Result valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const TokenList& tokenList, ErrorLogger& errorLogger, const Settings& settings)
{
if (Settings::terminated())
throw TerminateException();
Expand Down
4 changes: 2 additions & 2 deletions lib/forwardanalyzer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ Analyzer::Result valueFlowGenericForward(Token* start,
const Token* end,
const ValuePtr<Analyzer>& a,
const TokenList& tokenList,
ErrorLogger* const errorLogger,
ErrorLogger& errorLogger,
const Settings& settings);

Analyzer::Result valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const TokenList& tokenList, ErrorLogger* const errorLogger, const Settings& settings);
Analyzer::Result valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const TokenList& tokenList, ErrorLogger& errorLogger, const Settings& settings);

#endif
11 changes: 7 additions & 4 deletions lib/importproject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,20 +553,23 @@ namespace {
}
}

// see https://learn.microsoft.com/en-us/visualstudio/msbuild/msbuild-conditions
// properties are .NET String objects and you can call any of its members on them
bool conditionIsTrue(const ProjectConfiguration &p) const {
if (condition.empty())
return true;
std::string c = '(' + condition + ");";
replaceAll(c, "$(Configuration)", p.configuration);
replaceAll(c, "$(Platform)", p.platformStr);

// TODO: evaluate without using the Tokenizer
// TODO: improve evaluation
const Settings s;
Tokenizer tokenizer(s);
TokenList tokenlist(&s);
std::istringstream istr(c);
tokenizer.tokenize(istr,"vcxproj.c");
for (const Token *tok = tokenizer.tokens(); tok; tok = tok->next()) {
tokenlist.createTokens(istr, Standards::Language::C); // TODO: check result
for (const Token *tok = tokenlist.front(); tok; tok = tok->next()) {
if (tok->str() == "(" && tok->astOperand1() && tok->astOperand2()) {
// TODO: this is wrong - it is Contains() not Equals()
if (tok->astOperand1()->expressionString() == "Configuration.Contains")
return ('\'' + p.configuration + '\'') == tok->astOperand2()->str();
}
Expand Down
6 changes: 3 additions & 3 deletions lib/reverseanalyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,12 @@

namespace {
struct ReverseTraversal {
ReverseTraversal(const ValuePtr<Analyzer>& analyzer, const TokenList& tokenlist, ErrorLogger* const errorLogger, const Settings& settings)
ReverseTraversal(const ValuePtr<Analyzer>& analyzer, const TokenList& tokenlist, ErrorLogger& errorLogger, const Settings& settings)
: analyzer(analyzer), tokenlist(tokenlist), errorLogger(errorLogger), settings(settings)
{}
ValuePtr<Analyzer> analyzer;
const TokenList& tokenlist;
ErrorLogger* const errorLogger;
ErrorLogger& errorLogger;
const Settings& settings;

std::pair<bool, bool> evalCond(const Token* tok) const {
Expand Down Expand Up @@ -395,7 +395,7 @@ namespace {
};
}

void valueFlowGenericReverse(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const TokenList& tokenlist, ErrorLogger* const errorLogger, const Settings& settings)
void valueFlowGenericReverse(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const TokenList& tokenlist, ErrorLogger& errorLogger, const Settings& settings)
{
if (a->invalid())
return;
Expand Down
2 changes: 1 addition & 1 deletion lib/reverseanalyzer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,6 @@ class TokenList;
template<class T>
class ValuePtr;

void valueFlowGenericReverse(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const TokenList& tokenlist, ErrorLogger* const errorLogger, const Settings& settings);
void valueFlowGenericReverse(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const TokenList& tokenlist, ErrorLogger& errorLogger, const Settings& settings);

#endif
4 changes: 2 additions & 2 deletions lib/symboldatabase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2133,7 +2133,7 @@ void SymbolDatabase::validateExecutableScopes() const
for (std::size_t i = 0; i < functions; ++i) {
const Scope* const scope = functionScopes[i];
const Function* const function = scope->function;
if (scope->isExecutable() && !function) {
if (mErrorLogger && scope->isExecutable() && !function) {
const std::list<const Token*> callstack(1, scope->classDef);
const std::string msg = std::string("Executable scope '") + scope->classDef->str() + "' with unknown function.";
const ErrorMessage errmsg(callstack, &mTokenizer.list, Severity::debug,
Expand Down Expand Up @@ -2198,7 +2198,7 @@ void SymbolDatabase::debugSymbolDatabase() const
for (const Token* tok = mTokenizer.list.front(); tok != mTokenizer.list.back(); tok = tok->next()) {
if (tok->astParent() && tok->astParent()->getTokenDebug() == tok->getTokenDebug())
continue;
if (tok->getTokenDebug() == TokenDebug::ValueType) {
if (mErrorLogger && tok->getTokenDebug() == TokenDebug::ValueType) {

std::string msg = "Value type is ";
ErrorPath errorPath;
Expand Down
11 changes: 7 additions & 4 deletions lib/tokenize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1149,7 +1149,7 @@ void Tokenizer::simplifyTypedefCpp()
return;

if (maxTime > 0 && std::time(nullptr) > maxTime) {
if (mSettings.debugwarnings) {
if (mErrorLogger && mSettings.debugwarnings) {
ErrorMessage::FileLocation loc(list.getFiles()[0], 0, 0);
ErrorMessage errmsg({std::move(loc)},
emptyString,
Expand Down Expand Up @@ -3407,11 +3407,12 @@ bool Tokenizer::simplifyTokens1(const std::string &configuration)
const bool doValueFlow = !disableValueflowEnv || (std::strcmp(disableValueflowEnv, "1") != 0);

if (doValueFlow) {
assert(mErrorLogger);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After the change in ImportProject this can no longer happen. I did not change it to a reference yet since it used in a lot of redundant test code. As I plan to clean up that redundancy I will address it with that upcoming change instead of touching the code twice.

if (mTimerResults) {
Timer t("Tokenizer::simplifyTokens1::ValueFlow", mSettings.showtime, mTimerResults);
ValueFlow::setValues(list, *mSymbolDatabase, mErrorLogger, mSettings, mTimerResults);
ValueFlow::setValues(list, *mSymbolDatabase, *mErrorLogger, mSettings, mTimerResults);
} else {
ValueFlow::setValues(list, *mSymbolDatabase, mErrorLogger, mSettings, mTimerResults);
ValueFlow::setValues(list, *mSymbolDatabase, *mErrorLogger, mSettings, mTimerResults);
}

arraySizeAfterValueFlow();
Expand Down Expand Up @@ -3439,6 +3440,7 @@ bool Tokenizer::simplifyTokens1(const std::string &configuration)
return true;
}

// cppcheck-suppress unusedFunction - used in tests only
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was surprising but it actually foreshadows the cleanup I am planning to make.

bool Tokenizer::tokenize(std::istream &code,
const char FileName[],
const std::string &configuration)
Expand Down Expand Up @@ -6070,7 +6072,8 @@ void Tokenizer::dump(std::ostream &out) const
out << outs;
outs.clear();

mSymbolDatabase->printXml(out);
if (mSymbolDatabase)
mSymbolDatabase->printXml(out);

containers.erase(nullptr);
if (!containers.empty()) {
Expand Down
2 changes: 1 addition & 1 deletion lib/tokenize.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class CPPCHECKLIB Tokenizer {
friend class TemplateSimplifier;

public:
explicit Tokenizer(const Settings & settings, ErrorLogger *errorLogger = nullptr, const Preprocessor *preprocessor = nullptr);
explicit Tokenizer(const Settings & settings, ErrorLogger *errorLogger, const Preprocessor *preprocessor = nullptr);
~Tokenizer();

void setTimerResults(TimerResults *tr) {
Expand Down
Loading
Loading