Skip to content

Fix #13374 (Add --check-level=reduced option)#6496

Merged
danmar merged 5 commits into
cppcheck-opensource:mainfrom
cppchecksolutions:valueflow-fast-other
Dec 4, 2024
Merged

Fix #13374 (Add --check-level=reduced option)#6496
danmar merged 5 commits into
cppcheck-opensource:mainfrom
cppchecksolutions:valueflow-fast-other

Conversation

@danmar

@danmar danmar commented Jun 9, 2024

Copy link
Copy Markdown
Collaborator

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

danmar commented Jun 9, 2024

Copy link
Copy Markdown
Collaborator Author

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
Copy Markdown
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

danmar commented Jun 10, 2024

Copy link
Copy Markdown
Collaborator Author

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

danmar commented Jun 10, 2024

Copy link
Copy Markdown
Collaborator Author

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.

@danmar danmar force-pushed the valueflow-fast-other branch from c6c571b to 9325deb Compare June 23, 2024 03:02
@danmar danmar force-pushed the valueflow-fast-other branch 3 times, most recently from 44bf3c2 to b94ba63 Compare December 3, 2024 15:13
@danmar danmar marked this pull request as ready for review December 3, 2024 15:14
@danmar danmar changed the title valueflow-fast: Added a --check-level=fast option. Fix #13374 (Add --check-level=reduced option) Dec 3, 2024
@danmar danmar force-pushed the valueflow-fast-other branch from 443c6d6 to 41c442a Compare December 4, 2024 11:48
@danmar danmar force-pushed the valueflow-fast-other branch from 41c442a to cef3404 Compare December 4, 2024 14:48
@danmar danmar merged commit 07d59e9 into cppcheck-opensource:main Dec 4, 2024
@danmar danmar deleted the valueflow-fast-other branch December 5, 2024 09:32
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