Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix #11797 crash on missing/renamed files #5921

Closed
wants to merge 13 commits into from
Closed

Fix #11797 crash on missing/renamed files #5921

wants to merge 13 commits into from

Conversation

olabetskyi
Copy link
Collaborator

No description provided.

@firewave
Copy link
Collaborator

As usual - please add a unit test for this.

This looks related to #5411 which I still need to finish up.

Also didn't we already add such code? I remember having a discussion with @danmar about something similar (with me having quite some opinion on this).

@olabetskyi
Copy link
Collaborator Author

As usual - please add a unit test for this.

This looks related to #5411 which I still need to finish up.

Also didn't we already add such code? I remember having a discussion with @danmar about something similar (with me having quite some opinion on this).

It's not finished yet. Should have marked it as draft

@olabetskyi olabetskyi marked this pull request as draft January 29, 2024 15:18
@danmar
Copy link
Owner

danmar commented Jan 30, 2024

Also didn't we already add such code? I remember having a discussion with @danmar about something similar (with me having quite some opinion on this).

I don't know.. I had a vague feeling we added something like this. But we do reproduce crashes with latest git head.

danmar
danmar reviewed Jan 30, 2024
View reviewed changes
test/testsuppressions.cpp Outdated Show resolved Hide resolved
danmar
danmar reviewed Jan 30, 2024
View reviewed changes
lib/cppcheck.cpp Outdated Show resolved Hide resolved
@danmar
Copy link
Owner

danmar commented Jan 30, 2024

with me having quite some opinion on this

@firewave sorry but I don't have the discussion in my head.. please feel free to comment on this.

  • This approach will mean that "fileNotFound" errors are written in the gui + xml report which is desired behavior imho.
  • If the file is deleted/renamed for some reason after the project is imported then cppcheck will not crash but show the error report

danmar
danmar reviewed Jan 30, 2024
View reviewed changes
Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits..

lib/cppcheck.cpp Outdated Show resolved Hide resolved
lib/cppcheck.cpp Outdated Show resolved Hide resolved
lib/cppcheck.cpp Outdated Show resolved Hide resolved
danmar
danmar reviewed Jan 30, 2024
View reviewed changes
Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question

lib/cppcheck.cpp Outdated Show resolved Hide resolved
@olabetskyi olabetskyi marked this pull request as ready for review January 30, 2024 13:37
danmar
danmar approved these changes Jan 30, 2024
View reviewed changes
Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from my point of view I feel satisfied. @firewave please feel free to review..

@olabetskyi olabetskyi changed the title Fix issue with missing files listed in project Fix #11797 crash on missing/renamed files Jan 31, 2024
@firewave
Copy link
Collaborator

This is okay as a catch-all to avoid the crashes (which is the most important in the short-term) but I still think it should be caught earlier.

This needs a release notes entry.

We also need Python tests for each project input which is lacking a file. I do not feel like adding tests later on which should have been there in the first place - again.

danmar
danmar reviewed Jan 31, 2024
View reviewed changes
Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think test can be simplified.

test/cli/invalid-project/invalid-project.vcxproj Outdated Show resolved Hide resolved
@danmar
Copy link
Owner

danmar commented Jan 31, 2024

firewave has made an effort to make sure tests can be executed from different folders. As your test is written I fear it must be executed from inside test/cli folder.

@danmar
Copy link
Owner

danmar commented Jan 31, 2024

@firewave

but I still think it should be caught earlier.

Overall I think that it is a good strategy to fail immediately/early... But we should probably think more carefully about how we report problems. If we just report it directly in plain text on stdout/stderr then it is likely that cppcheck plugins will not accept it as expected output - the user might not get any information about what goes wrong. We also need to ensure that the problem is properly logged in the GUI.

@olabetskyi olabetskyi force-pushed the project_file branch 2 times, most recently from 6d8c0aa to 623cc0d Compare February 2, 2024 15:00
olabetskyi
olabetskyi commented Feb 2, 2024
View reviewed changes
lib/cppcheck.h Outdated Show resolved Hide resolved
danmar
danmar reviewed Aug 26, 2024
View reviewed changes
lib/cppcheck.cpp Outdated Show resolved Hide resolved
@olabetskyi olabetskyi force-pushed the project_file branch 2 times, most recently from 8b04daa to 3119e09 Compare August 30, 2024 14:35
danmar
danmar reviewed Aug 30, 2024
View reviewed changes
lib/cppcheck.cpp Outdated Show resolved Hide resolved
lib/cppcheck.cpp Show resolved Hide resolved
lib/cppcheck.cpp Outdated Show resolved Hide resolved
test/cfg/emscripten.cpp Outdated Show resolved Hide resolved
test/cli/more-projects_test.py Outdated Show resolved Hide resolved
test/cli/shared-items-project/Main/MainFile.cpp Outdated Show resolved Hide resolved
test/cli/shared-items-project/Shared/TestClass.h Outdated Show resolved Hide resolved
test/cli/whole-program/odr2.cpp Outdated Show resolved Hide resolved
danmar
danmar reviewed Aug 30, 2024
View reviewed changes
lib/cppcheck.cpp Show resolved Hide resolved
lib/cppcheck.cpp Show resolved Hide resolved
@olabetskyi
Copy link
Collaborator Author

olabetskyi commented Aug 30, 2024 •
edited by danmar
Loading

Crash was fixed with Simplecpp Fix and than bumped.

@olabetskyi olabetskyi closed this Aug 30, 2024
@olabetskyi olabetskyi deleted the project_file branch August 30, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danmar danmar danmar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@olabetskyi @firewave @danmar