◐ Shell
reader mode source ↗
Skip to content

added (optional) lazy execution of ValueFlow#4521

Draft
firewave wants to merge 4 commits into
cppcheck-opensource:mainfrom
firewave:vf
Draft

added (optional) lazy execution of ValueFlow#4521
firewave wants to merge 4 commits into
cppcheck-opensource:mainfrom
firewave:vf

Conversation

@firewave

Copy link
Copy Markdown
Collaborator

No description provided.

@firewave firewave force-pushed the vf branch 3 times, most recently from 2b93752 to 07ba92f Compare September 29, 2022 13:26
@pfultz2

pfultz2 commented Oct 4, 2022

Copy link
Copy Markdown
Contributor

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.

@firewave

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.

@firewave

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.

@firewave

Copy link
Copy Markdown
Collaborator Author

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

@firewave

Copy link
Copy Markdown
Collaborator Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants