-
Notifications
You must be signed in to change notification settings - Fork 1.5k
improved determination if application is Premium / added TODOs #7346
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
base: main
Are you sure you want to change the base?
Conversation
| const std::string manualUrl(premium ? | ||
| "https://files.cppchecksolutions.com/manual.pdf" : | ||
| "https://cppcheck.sourceforge.io/manual.pdf"); |
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 were switched.
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.
Also we should not have the custom URL hardcoded and read it from cppcheck.cfg like the product name instead. I files https://trac.cppcheck.net/ticket/14208 about this.
|
This is in preparation of avoiding It also gets rid of loading the |
test/cli/premium_test.py
Outdated
| "productName": "NAME", | ||
| "about": "NAME", | ||
| "safety": true | ||
| "about": "NAME" |
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 test was based on the behavior that is loaded cppcheck.cfg first and being overriden by the CLI. But that is not how it was initially implemented. That behavior changed when the double loading was introduced in #5760.
b093636 to
7c9b217
Compare
| exitcode, _, stderr = cppcheck(['--xml-version=3', '--premium=safety-off', test_file], cppcheck_exe=exe) | ||
| exitcode, _, stderr = cppcheck(['--xml-version=3', test_file], cppcheck_exe=exe) | ||
| assert exitcode == 0 | ||
| assert '<safety/>' not in stderr |
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.
Cppcheck Premium should enable safety by default. So I expect <safety/> in xml output unless it's explicitly turned off with --premium=safety-off.
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.
Cppcheck Premium should enable safety by default.
That means that it is explicitly enabled in cppcheck.cfg, right? Should it even be allowed to be disabled by default if it is premium (i.e. <safety> is missing or set to false)?
What about --no-safety? May it also override the premium safety (default)? Or should that be limited to --premium=safety-off?
I would be fine having special handling for the safety flag. I just would have it properly defined.
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.
@danmar I need feedback on this to proceed.
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.
@danmar Still need feedback to be able to proceed.
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.
That means that it is explicitly enabled in cppcheck.cfg, right?
yes.
Should it even be allowed to be disabled by default if it is premium (i.e. is missing or set to false)?
We can allow that it's off.
If the user provides --safety or --safety-off on the command line then those options should override what the cppcheck.cfg says.
imho if there is missing safety option in cppcheck.cfg then safety should be off by default.
my intention is that cppcheck.cfg is a general purpose interface so that everybody can customize cppcheck if they want to, maybe somebody wants that cppcheck will always execute certain extra addons.. there should be a minimum of cppcheck premium specific code in open source cppcheck.
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.
@danmar Still need feedback to proceed.
FYI If this weren't premium-related I would have just defined things and merged it by now.
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.
Thanks, I will get rid of any special handling.
That also makes safety within --premium redundant and I think we should deprecate/remove it.
| exitcode, _, stderr = cppcheck(['--premium=autosar', '--xml', test_file], cppcheck_exe=exe) | ||
| assert exitcode == 0 | ||
| assert 'id="unusedVariable"' in stderr | ||
| assert 'id="checkersReport"' not in stderr |
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 expect to see checkersReport in the Cppcheck Premium output unless --premium=safety-off is provided.
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.
That is because the config used in the test doesn't have safety set. The changes in this PR removed that it is always implicitly enabled.
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.
Actually I removed it in the changes and inverted the checks. I reverted that change.
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.
The actual problem is that cppcheck.cfg is read after the CLI options because of the assumption that the config should override those.
d342511 to
4979097
Compare
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.
sorry I had this review comment pending.. hope it helps
| exitcode, _, stderr = cppcheck(['--xml-version=3', '--premium=safety-off', test_file], cppcheck_exe=exe) | ||
| exitcode, _, stderr = cppcheck(['--xml-version=3', test_file], cppcheck_exe=exe) | ||
| assert exitcode == 0 | ||
| assert '<safety/>' not in stderr |
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.
That means that it is explicitly enabled in cppcheck.cfg, right?
yes.
Should it even be allowed to be disabled by default if it is premium (i.e. is missing or set to false)?
We can allow that it's off.
If the user provides --safety or --safety-off on the command line then those options should override what the cppcheck.cfg says.
imho if there is missing safety option in cppcheck.cfg then safety should be off by default.
my intention is that cppcheck.cfg is a general purpose interface so that everybody can customize cppcheck if they want to, maybe somebody wants that cppcheck will always execute certain extra addons.. there should be a minimum of cppcheck premium specific code in open source cppcheck.
|
Looks like this is already complete. I did not check if the non-premium and premium CLI flags are correctly working with each other. I think it is fine to assume that they are currently not being mixed and if we deprecate one set of them that should also not be an issue going forward. |
b49b051 to
e9acdd4
Compare
This comment was marked as resolved.
This comment was marked as resolved.
6ac9ee0 to
aabc350
Compare
|
|
Anything still to do here? I have several follow-ups lined up. |
| return false; | ||
| } | ||
|
|
||
| settings.premium = startsWith(settings.cppcheckCfgProductName, "Cppcheck Premium"); |
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.
it feels like this line of code belongs in LIB?



No description provided.