introduced a cache for followAllReferences() calls with default parameters
#7192
Conversation
|
This essentially eliminates any meaningful impact by
Clang 19
The example from https://trac.cppcheck.net/ticket/10765#comment:4: Clang 19
|
lib/valueflow.cpp
Outdated
| @@ -3738,7 +3738,7 @@ static void valueFlowForwardConst(Token* start, | |||
| } else { | |||
| [&] { | |||
| // Follow references | |||
| auto refs = followAllReferences(tok); | |||
| auto refs = tok->refs(); | |||
There was a problem hiding this comment.
needs to be const auto&.
There was a problem hiding this comment.
Done.
I filed https://trac.cppcheck.net/ticket/13533 about detecting this.
lib/vf_analyzers.cpp
Outdated
| @@ -627,7 +627,7 @@ struct ValueFlowAnalyzer : Analyzer { | |||
| if (invalid()) | |||
| return Action::Invalid; | |||
| // Follow references | |||
| auto refs = followAllReferences(tok); | |||
| auto refs = tok->refs(); | |||
There was a problem hiding this comment.
This copy is necessary since an additional entry is being added. But I think this is not necessary and I will try to refactor the code to avoid it.
There was a problem hiding this comment.
I adjusted this by unfortunately there is some redundant code introduced.
| if (!mImpl->mRefs) | ||
| mImpl->mRefs = new SmallVector<ReferenceToken>(followAllReferences(this)); | ||
| return *mImpl->mRefs; | ||
| } |
There was a problem hiding this comment.
This is not the right way to do this. This is a const method that is modifying the token. Instead followAllReferences should be moved to the SymbolDatabase and there should be a pass that fills this in for all of the tokens.
There was a problem hiding this comment.
Yes. I was about to add a comment about this. This violates the technical const and if we would not allow this (I hope some day I will finish up that change) this would require mutable (which from my experience is acceptable for caches inside objects).
I am not sure how easy it would be to implement an earlier pass since it is not done for all tokens but there are lots of checks which are performed before we actually end up following references. That would need to be replicated I reckon - and that also has a certain visible overhead and we would need to run through that twice then.
Actually I would also have the ValueFlow behave this way so we might avoid running it for code which is not relevant.
There was a problem hiding this comment.
This is not the right way to do this. This is a
constmethod that is modifying the token.
That should be totally fine (by precedent). We modify const Token objects all over the place in the ValueFlow and symbol database via const_cast. Obviously it would be better if we didn't but here it is much cleaner and in a single place and as stated before I think this is acceptable practice.
Actually I would also have the ValueFlow behave this way so we might avoid running it for code which is not relevant.
Please disregard this. This is wishful thinking as this would not be possible the way the ValueFlow is working. I totally forgot I already looked into this.
| @@ -143,6 +145,8 @@ struct TokenImpl { | |||
| void setCppcheckAttribute(CppcheckAttributes::Type type, MathLib::bigint value); | |||
| bool getCppcheckAttribute(CppcheckAttributes::Type type, MathLib::bigint &value) const; | |||
|
|
|||
| SmallVector<ReferenceToken>* mRefs{}; | |||
There was a problem hiding this comment.
If this is going to be a pointer, you should use std::unique_ptr or std::shared_ptr.
There was a problem hiding this comment.
I modeled it after mValues which is also just a raw pointer.
|
Something really weird is going on here in the UBSAN job: The timing information for |
|
Before After |
firewave
force-pushed
the
followref-cache
branch
from
42d1ec9 to
b38e8ba
Compare
firewave
force-pushed
the
followref-cache
branch
from
b38e8ba to
a36380e
Compare
No description provided.