-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix #13440 (crash if there is exception in whole program analysis) #7113
base: main
Are you sure you want to change the base?
Conversation
@firewave any opinion about this? If we catch exception here then other addons can still be executed. It feels like it could be a good idea to catch exceptions in |
try { | ||
results = executeAddon(addonInfo, mSettings.addonPython, fileList.empty() ? files[0] : fileList, mSettings.premiumArgs, mExecuteCommand); | ||
} catch (const InternalError& e) { | ||
const ErrorMessage errmsg = ErrorMessage::fromInternalError(e, nullptr, file0, "Bailing out from analysis: Addon '" + addonInfo.name + "' failed"); |
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 name we have in addonInfo.name looks like a placeholder
Judging from the failing tests it looks like this should be already handled and there are already tests for this. I will have a look later. |
@@ -358,3 +358,18 @@ def test_checkclass_project_builddir_j(tmpdir): | |||
build_dir = os.path.join(tmpdir, 'b1') | |||
os.mkdir(build_dir) | |||
__test_checkclass_project(tmpdir, ['-j2', '--cppcheck-build-dir={}'.format(build_dir)]) | |||
|
|||
|
|||
def test_ctu_fail(tmpdir): |
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 file only contain tests which relate to the whole-program
folder so this should be added the other_test.py
.
The whole exception handling in that area needs to be done a bit differently. But it is something which still needs some tests to be written. And I did not want to touch it until the common path works actually works as intended (see my current and upcoming analyzer information related PRs). |
@firewave nevermind I will have to take another look at this. please ignore it for now. |
I am looking at it right now because there are some gaps in the testing. |
ok. thanks. |
Ah - I missed the The change was fine and just needed some fine-tuning. I published #7114 as an alternative. |
No description provided.