fixed #13982 - cppcheck.cpp: do not crash if mExecuteCommand is empty
#7647
Conversation
firewave
changed the title
mExecuteCommand is emptymExecuteCommand is empty
|
This only fixes the crash when running an scratchpad analysis with a loaded projects including addons. It will now show an error instead. The execute function is not available in that context and it requires some refactoring. But I think that is something to look into in the context of https://trac.cppcheck.net/ticket/13976. |
This comment was marked as resolved.
This comment was marked as resolved.
firewave
force-pushed
the
scratch-addon
branch
4 times, most recently
from
988764d to
e6a6e24
Compare
|
I moved the checks from the executor tests as the code being tested was not specific to those classes and actually lives in |
| @@ -686,6 +689,11 @@ unsigned int CppCheck::checkClang(const FileWithDetails &file, int fileIndex) | |||
| mErrorLogger.reportOut(exe + " " + args2, Color::Reset); | |||
| } | |||
|
|
|||
| if (!mExecuteCommand) { | |||
| std::cerr << "Failed to execute '" << exe << " " << args2 << " " << redirect2 << "' - (no command callback provided)" << std::endl; | |||
There was a problem hiding this comment.
why isn't this reported to the errorlogger. GUI does not capture stderr. And plugins will probably not write this. So the user will have no clue why it does not work.
There was a problem hiding this comment.
Because that was never properly implemented for this. It's the same in another cases and there's TODOs in the code. Will check if we have tickets about this.
There was a problem hiding this comment.
I don't understand what I am missing. What is stopping you from calling mErrorLogger.reportError here instead?
There was a problem hiding this comment.
I don't understand what I am missing. What is stopping you from calling
mErrorLogger.reportErrorhere instead?
It just was never implemented that way back when this functionality was introduced. That is outside of the scope of this PR.
There was a problem hiding this comment.
If we write this code instead:
const std::list<ErrorMessage::FileLocation> callstack{
{file.spath(), -1, 0}
};
const ErrorMessage msg(callstack, nullptr, Severity::error, "internalError", "Failed to execute clang, mExecuteCommand is null", Certainty::normal);
mErrorLogger.reportErr(msg);
It still compiles. I don't ask that you change other std::cerr/std::cout statements throughout the code, those should be fixed but I agree that is out of scope.
There was a problem hiding this comment.
maybe a throw InternalError is easier.. your choice.
There was a problem hiding this comment.
I will file tickets and address this in a follow-up.
Great - this is back. Now any build will show up as failed after it has been merged... |
| @@ -432,6 +432,9 @@ static std::vector<picojson::value> executeAddon(const AddonInfo &addonInfo, | |||
| const std::string &premiumArgs, | |||
| const CppCheck::ExecuteCmdFn &executeCommand) | |||
| { | |||
| if (!executeCommand) | |||
| throw InternalError(nullptr, "Failed to execute addon - no command callback provided"); | |||
There was a problem hiding this comment.
how about writing it more detailed..
Cannot execute addon, function pointer 'executeCommand' is null
| @@ -686,6 +689,11 @@ unsigned int CppCheck::checkClang(const FileWithDetails &file, int fileIndex) | |||
| mErrorLogger.reportOut(exe + " " + args2, Color::Reset); | |||
| } | |||
|
|
|||
| if (!mExecuteCommand) { | |||
| std::cerr << "Failed to execute '" << exe << " " << args2 << " " << redirect2 << "' - (no command callback provided)" << std::endl; | |||
There was a problem hiding this comment.
If we write this code instead:
const std::list<ErrorMessage::FileLocation> callstack{
{file.spath(), -1, 0}
};
const ErrorMessage msg(callstack, nullptr, Severity::error, "internalError", "Failed to execute clang, mExecuteCommand is null", Certainty::normal);
mErrorLogger.reportErr(msg);
It still compiles. I don't ask that you change other std::cerr/std::cout statements throughout the code, those should be fixed but I agree that is out of scope.
| @@ -686,6 +689,11 @@ unsigned int CppCheck::checkClang(const FileWithDetails &file, int fileIndex) | |||
| mErrorLogger.reportOut(exe + " " + args2, Color::Reset); | |||
| } | |||
|
|
|||
| if (!mExecuteCommand) { | |||
| std::cerr << "Failed to execute '" << exe << " " << args2 << " " << redirect2 << "' - (no command callback provided)" << std::endl; | |||
There was a problem hiding this comment.
maybe a throw InternalError is easier.. your choice.
we have several successful builds. if the configuration will need adjustments I am open to suggestions. I like to get more warnings but my feeling is that sonar is a lot more noisy. but I haven't seen any failed builds because of sonarcloud since yesterday at least. |
|
This should have been in the release... Will rebase later so it can be merged. Unless thee is still something to do. |
… shadowed a base member variable)
|
| if (!mExecuteCommand) { | ||
| std::cerr << "Failed to execute '" << exe << "' (no command callback provided)" << std::endl; | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Reporting a proper error is already tracked in https://trac.cppcheck.net/ticket/13827.
No description provided.