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.

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

valueflow-fast: Added a --check-level=fast option. #6496

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

danmar
Copy link
Owner

@danmar danmar commented Jun 9, 2024

No description provided.

@danmar danmar requested a review from firewave June 9, 2024 06:26
@danmar danmar marked this pull request as draft June 9, 2024 06:26
@danmar danmar changed the title valueflow-fast: Added a --check-level=fast option. Run valueflow with only 1 iteration. valueflow-fast: Added a --check-level=fast option. Jun 9, 2024
@danmar
Copy link
Owner Author

danmar commented Jun 9, 2024

On the testfiles that a customer had special problems with.. when --check-level=fast is used, valueflow does not take longer time than parsing. I believe this will be acceptable. If significantly faster analysis is required then we'll also need to look at speeding up the parsing.

@firewave
Copy link
Collaborator

Just tuning the values seems more reasonable to me than providing a different implementation.

But I think we should implement bailout (debug) messages (at least for all the ones used here) so it is possible to actually see the impact this option has.

I still fell that fast is misleading. Obviously people want it fast but it should rather reflect what it actually does like reduced. That seems like a better opposite to the exhaustive (which is also misleading because it was actually the "normal" - I would prefer something like full for that).

Maybe we should not add this configuration at all but expose all the values via cppcheck.cfg. So in conjunction with the bailout messages the customer could increase certain values closer to the normal not impacting the Valueflow coverage as much.

@danmar
Copy link
Owner Author

danmar commented Jun 10, 2024

For your information I asked these 4 questions to the customer that this valueflow-fast is implemented for. I'm not saying that this is the one and only opinion that matters just that we can try to be flexible..

(1) If you enable --check-level=fast do you want that warnings about bailouts are written?

Not by default but it could be interesting to see if the warning is opt-in (e.g. informational severity). Once Cppcheck is tuned for a project such warnings are noise.

(2) Do you want to have a --check-level=fast that we try to make sufficiently fast (by guessing more or less) or would you like fine grained options that controls how many flow-iterations we perform, how many branches we perform, etc.

Simpler is generally better and the 'fast' level selection is what we'd make available to project teams. However, fine-grained options would allow us to tune what 'fast' means or define a couple of levels of 'fast'. Either way it would be good to have an way to see, for example, what percentage of branches are checked, and some guidance as a starting point for tuning.

(3) if you have fine grained options for the data flow, would you prefer to provide these on the command line or in a file?

Command line, unless OS-level command line length limits would be exceeded.

(4) Do I understand the use case properly that you want to have a local run that is fast. And when executing in the CI you will use the "exhaustive" analysis. Or will "fast" be used in the CI also?

Depends on the project, Local developer builds, per-patch CI, and nightly/release CI builds have different performance expectations.

@danmar
Copy link
Owner Author

danmar commented Jun 10, 2024

I still fell that fast is misleading. Obviously people want it fast but it should rather reflect what it actually does like reduced.

I agree "fast" is not a good name.

Maybe we should not add this configuration at all but expose all the values via cppcheck.cfg.

I would not put it in cppcheck.cfg that is supposed to be global options that are used every time. But well such configuration can be added in a file.

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