GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — added (optional) lazy execution of ValueFlow by firewave · Pull Request #4521 · cppcheck-opensource/cppcheck

Conversation

firewave commented Sep 29, 2022

Copy link

Copy Markdown

Collaborator

No description provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

firewave force-pushed the vf branch 3 times, most recently from 2b93752 to 07ba92f Compare

September 29, 2022 13:26

pfultz2 reviewed

Oct 4, 2022

View reviewed changes

Comment thread

lib/tokenlist.h


		std::function<void()> mValuesFunc;
		mutable bool mValuesSet;
		mutable bool mInSetValues;

pfultz2 Oct 4, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

These two variables can be moved into the mValuesFunc.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pfultz2 Oct 4, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We could do something similar to what we do for memoize:

template<class F>
static std::function<void()> once(F f)
{
    bool ran = false;
    return [=]() mutable {
        if (ran)
            return;
        ran = true;
        f();
    };
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

firewave Oct 4, 2022

Copy link

Copy Markdown

Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This was very hacky. I just tried to get rid what I wanted it to do.

These two variables can be moved into the mValuesFunc.

Good point.

We could do something similar to what we do for [memoize]

Will take a look. I know there are patterns for this but I rarely use them so I keep forgetting them.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pfultz2 reviewed

Oct 4, 2022

View reviewed changes

Comment thread

lib/token.cpp

		const ValueFlow::Value* Token::getContainerSizeValue(const MathLib::bigint val) const
		{
		if (!mImpl->mValues)
		if (values().empty())

pfultz2 Oct 4, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

values().empty() is redundant here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

firewave Oct 4, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I kept it to avoid the function calls (which obviously won't do much) since some of these might be called loooooots of times. I haven't profiled any of this yet.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

firewave Aug 8, 2023

Copy link

Copy Markdown

Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I would not call it "redundant" but "unnecessary". Still needs to be profiled.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pfultz2 reviewed

Oct 4, 2022

View reviewed changes

Comment thread

lib/token.cpp

		const ValueFlow::Value* Token::getMovedValue() const
		{
		if (!mImpl->mValues)
		if (values().empty())

pfultz2 Oct 4, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

values().empty() is redundant here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pfultz2 reviewed

Oct 4, 2022

View reviewed changes

Comment thread

lib/token.cpp

		const ValueFlow::Value* Token::getMaxValue(bool condition, MathLib::bigint path) const
		{
		if (!mImpl->mValues)
		if (values().empty())

pfultz2 Oct 4, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

values().empty() is redundant here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pfultz2 reviewed

Oct 4, 2022

View reviewed changes

Comment thread

lib/token.cpp

		const ValueFlow::Value* Token::getValue(const MathLib::bigint val) const
		{
		if (!mImpl->mValues)
		if (values().empty())

pfultz2 Oct 4, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

values().empty() is redundant here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pfultz2 reviewed

Oct 4, 2022

View reviewed changes

Comment thread

lib/token.cpp

		return false;
		return mImpl->mValues &&
		std::any_of(mImpl->mValues->begin(), mImpl->mValues->end(), [&](const ValueFlow::Value& value) {
		return !values().empty() &&

pfultz2 Oct 4, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

values().empty() is redundant here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pfultz2 reviewed

Oct 4, 2022

View reviewed changes

Comment thread

lib/token.cpp

		bool Token::hasKnownValue() const
		{
		return mImpl->mValues && std::any_of(mImpl->mValues->begin(), mImpl->mValues->end(), std::mem_fn(&ValueFlow::Value::isKnown));
		return !values().empty() && std::any_of(values().begin(), values().end(), std::mem_fn(&ValueFlow::Value::isKnown));

pfultz2 Oct 4, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

values().empty() is redundant here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pfultz2 reviewed

Oct 4, 2022

View reviewed changes

Comment thread

lib/token.cpp

		bool Token::hasKnownIntValue() const
		{
		if (!mImpl->mValues)
		if (values().empty())

pfultz2 Oct 4, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

values().empty() is redundant here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pfultz2 reviewed

Oct 4, 2022

View reviewed changes

Comment thread

lib/token.cpp

		const Token *Token::getValueTokenMaxStrLength() const
		{
		if (!mImpl->mValues)
		if (values().empty())

pfultz2 Oct 4, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

values().empty() is redundant here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pfultz2 reviewed

Oct 4, 2022

View reviewed changes

Comment thread

lib/token.cpp

		const Token Token::getValueTokenMinStrSize(const Settings settings, MathLib::bigint* path) const
		{
		if (!mImpl->mValues)
		if (values().empty())

pfultz2 Oct 4, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

values().empty() is redundant here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pfultz2 reviewed

Oct 4, 2022

View reviewed changes

Comment thread

lib/token.cpp

		const ValueFlow::Value * Token::getInvalidValue(const Token ftok, nonneg int argnr, const Settings settings) const
		{
		if (!mImpl->mValues \|\| !settings)
		if (values().empty() \|\| !settings)

pfultz2 Oct 4, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

values().empty() is redundant here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pfultz2 reviewed

Oct 4, 2022

View reviewed changes

Comment thread

lib/token.cpp

		const ValueFlow::Value * Token::getValueGE(const MathLib::bigint val, const Settings *settings) const
		{
		if (!mImpl->mValues)
		if (values().empty())

pfultz2 Oct 4, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

values().empty() is redundant here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

I think its a lot cleaner to use values() instead of mImpl->mValues. However, !mImp->mValues is not the equivalent of !values.empty(). If we use values() then the checks for null can be removed.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

firewave commented Oct 4, 2022

Copy link

Copy Markdown

Collaborator Author

I think its a lot cleaner to use values() instead of mImpl->mValues.

That was my takeaway as well after I "cracked" this and saw only one method needs to access the raw pointers - which is quite nice. I will extract the encapsulation and cleanup parts of this into a separate PR but it still needs some work (see below).

However, !mImp->mValues is not the equivalent of !values.empty(). If we use values() then the checks for null can be removed.

I am aware of that. I already fixed up a few mistakes I made in earlier revisions.

But it is still very much WIP...

I still have to profile this with valueflow enabled and disabled.

Also the lazy execution doesn't save anything at all.
Only when used in conjunction with #4362 it will actually do something since there's currently no way to control specific checks and even the special ones don't do that as evident by the above PR and several tickets I filed. This is a bigger thing and quite a can of worms but I have some ideas on how to incrementally address this to enhance the control and get rid of my hacks used for profiling and the CI.
I was hoping that maybe some checks can be adjusted to "delay" usage of the vaueflow data so we could profit in more cases but without proper control about what check is run it won't do much in the end.

There's also some tests failing with the lazy execution. But I guess they just lack the lazy execution hook. Still it is interesting and maybe we should make valueflow more explicit in tests so we know what depends on it. It might also help with determining how to increase the test coverage. But I haven't looked into this at all.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

firewave added 4 commits

October 5, 2022 11:11


          Token: avoid unnecessary direct access of TokenImpl::mValues

dfa2fb3


          moved TokensFrontBack from token.h to tokenlist.h

c4f1685


          tokenize.cpp: small refactoring

63e12eb


          added lazy execution of ValueFlow via DISABLE_VALUEFLOW=2 environme…

58dcddb

…nt variable

firewave force-pushed the vf branch from 07ba92f to 58dcddb Compare

October 5, 2022 09:19

firewave mentioned this pull request

Apr 17, 2023

Add a --check-level option #4942

Merged

firewave commented Aug 8, 2023

Copy link

Copy Markdown

Collaborator Author

This should "help" (as in executing less code) with tests which use the Tokenizer but do not rely on the ValueFlow. That might also fix the issue I hit in #5299.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

firewave mentioned this pull request

Sep 1, 2023

avoid duplicated workflow invocations for pushes in PRs made from root repository branches #5377

Merged

firewave mentioned this pull request

Oct 13, 2023

moved TokensFrontBack from token.h to tokenlist.h #5547

Merged

firewave mentioned this pull request

Jan 5, 2024

enabled and mitigated debug warnings in cfg tests #5840

Merged

firewave mentioned this pull request

Apr 19, 2024

Add --check-level=fast option that reuses much faster valueflow analysis from 1.90 #6097

Closed

firewave commented Apr 20, 2024

Copy link

Copy Markdown

Collaborator Author

I will try to pull out the wrapper function for values so this can finally progress.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

firewave commented Oct 31, 2025

Copy link

Copy Markdown

Collaborator Author

I filed https://trac.cppcheck.net/ticket/14243 about this.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Reviewers

pfultz2

pfultz2 left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

firewave commented Sep 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfultz2 Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

firewave Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfultz2 commented Oct 4, 2022

Uh oh!

firewave commented Oct 4, 2022

Uh oh!

firewave commented Aug 8, 2023

Uh oh!

firewave commented Apr 20, 2024

Uh oh!

firewave commented Oct 31, 2025

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pfultz2 Oct 4, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

firewave Oct 4, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.