ValueFlow: pass ErrorLogger by reference into ValueFlow::setValues() / removed need for LifetimeStore::Context
#5299
Conversation
|
Needs some reworking around |
firewave
force-pushed
the
ptr-ref-v
branch
4 times, most recently
from
e7bc1ff
to
ed3cb49
Compare
firewave
changed the title
ErrorLogger by reference into ValueFlow::setValues()ErrorLogger by reference into ValueFlow::setValues() / removed need for LifetimeStore::Context
firewave
force-pushed
the
ptr-ref-v
branch
4 times, most recently
from
d0d4904
to
38c8092
Compare
| : analyzer(analyzer), tokenList(tokenList), errorLogger(errorLogger), settings(settings) | ||
| {} | ||
| ValuePtr<Analyzer> analyzer; | ||
| const TokenList& tokenList; | ||
| ErrorLogger* const errorLogger; | ||
| ErrorLogger& errorLogger; |
There was a problem hiding this comment.
This makes the class not assignable. This should be a pointer.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
lib/valueflow.cpp
Outdated
| mContext->errorLogger = errorLogger; | ||
| mContext->settings = &settings; | ||
| } | ||
| mutable Token* forwardTok{}; |
There was a problem hiding this comment.
I don't know about using the mutable keyword here.
There was a problem hiding this comment.
Me neither - but removing the const from the function had quite a ripple effect before - will check again. I think this might be a sensible use of mutable.
There was a problem hiding this comment.
The ripple effect wasn't as bad as I remember. The object was only const when passing it around so that was not fully enforced and is actually fine.
firewave
force-pushed
the
ptr-ref-v
branch
4 times, most recently
from
111292c
to
53dd484
Compare
| @@ -3408,11 +3408,12 @@ bool Tokenizer::simplifyTokens1(const std::string &configuration) | |||
| const bool doValueFlow = !disableValueflowEnv || (std::strcmp(disableValueflowEnv, "1") != 0); | |||
|
|
|||
| if (doValueFlow) { | |||
| assert(mErrorLogger); | |||
There was a problem hiding this comment.
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.
| @@ -3440,6 +3441,7 @@ bool Tokenizer::simplifyTokens1(const std::string &configuration) | |||
| return true; | |||
| } | |||
|
|
|||
| // cppcheck-suppress unusedFunction - used in tests only | |||
There was a problem hiding this comment.
This was surprising but it actually foreshadows the cleanup I am planning to make.
firewave
force-pushed
the
ptr-ref-v
branch
2 times, most recently
from
8eac401
to
39369b4
Compare
…initionGroup::conditionIsTrue()`
No description provided.