-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
greatly improved Settings::loadCppcheckCfg()
error handling
#5712
Conversation
Requires #5704 to be merged first. |
1b827e0
to
6ef6767
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.
lgtm, thanks for making the loading cppcheck.cfg more robust.
…to latest dev version (#5710) The lastest release of picojson does not support creation of JSONs, so we need to switch to the current dev version.
The tests which are failing were in introduced in #5712. Those were not included in #5710 which updated `picojson` that resulted in the different lines being reported in the error messages. I also added some checks to `ScopeFile` which will indicate that a temporary file already exists highlighting multi-threading issues and leftover files from previously aborted testruns. I ran into his while looking into these failing tests
mSettings.loadCppcheckCfg(); | ||
return startsWith(mSettings.cppcheckCfgProductName, "Cppcheck Premium"); | ||
bool CmdLineParser::isCppcheckPremium() { | ||
Settings settings; |
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.
@firewave May I ask you why do we need to create the local settings here?
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.
So we do not accidentally load the config multiple times times. That should be done in a deterministic way once.
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.
Okay agree on that one. But you only creating local variable so it's not working.
adding static
to it will help though.
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.
But you only creating local variable so it's not working.
What is not working? We only need to compare s single string from the object. Also this is only done during the parsing of the command-line options.
adding
static
to it will help though.
What? The method or the object?
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.
What? The method or the object?
Both but as you've already marked the method you need to add it only here.
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.
Both but as you've already marked the method you need to add it only here.
On the object it would not help as it still requires the function call.
I think I did look at the method but I am not sure. It is also just a limited usage.
#endif | ||
// cppcheck-suppress knownConditionTrueFalse | ||
if (fileName.empty()) { | ||
fileName = Path::getPathFromFilename(exename) + cfgFilename; |
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.
What is not working? We only need to compare s single string from the object. Also this is only done during the parsing of the command-line options.
loadCppcheckCfg()
use member variable exename
for storing relative path and it's empty if you create setting locally
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.
loadCppcheckCfg()
use member variableexename
for storing relative path and it's empty if you create setting locally
Oh right. That again. I ran into his in another context as well. Will take a look later today.
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.
If I may. I'm already working on it in a separate ticket and was just asking you to help me clarify your thought process behind this particular 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.
I see. I would have just hot fixed this this for now.
As it would have also tied into my upcoming changes to fix https://trac.cppcheck.net/ticket/12240 and #4424 I would have done any more refactoring within that.
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.
Okay hot fix it is
This also fixes the issue that
cppcheck.cfg
is no longer being loaded from executable path. That was introduced by #5704.