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.

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

Make ofstreams thow exception on write issues #5255

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

samopolacek
Copy link
Contributor

@samopolacek samopolacek commented Jul 20, 2023

This is related to #5254.

I have observed that if non-existent --cppcheck-build-dir is used, all file writes silently fail.

Should we enable exceptions on std::ofstream(s)?

samopolacek

This comment was marked as resolved.

@samopolacek
Copy link
Contributor Author

@chrchr-github can you please rerun the failed check?

@samopolacek samopolacek marked this pull request as draft July 25, 2023 08:06
lib/cppcheck.cpp Outdated Show resolved Hide resolved
@samopolacek samopolacek marked this pull request as ready for review July 25, 2023 19:49
@samopolacek
Copy link
Contributor Author

Ready for review. A bit bloaty, not sure if all the asserts are needed, maybe I am too defensive. Please note that there are a few TODOs I would like to get reviewed.

{
assert(false);

// TODO report error
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure how

{
assert(false);

// TODO: Report error
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure how

{
assert(false);

// TODO report error
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure how

@firewave
Copy link
Collaborator

In some cases this is by design and would cause existing installations to fail. So this might require a soft deprecation with introducing a warning first in some cases.

Also we should add cli Python tests for each of these to make sure they fail as expected.

@@ -266,7 +281,10 @@ int CppCheckExecutor::check_internal(CppCheck& cppcheck)
mLatestProgressOutputTime = std::time(nullptr);

if (!settings.outputFile.empty()) {
mErrorOutput = new std::ofstream(settings.outputFile);
mErrorOutput = new std::ofstream();
mErrorOutput->exceptions(std::ios_base::failbit | std::ios_base::badbit);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of repeating this pattern across we should introduce a helper function for this.

cli/cppcheckexecutor.cpp Outdated Show resolved Hide resolved
@samopolacek samopolacek marked this pull request as draft July 26, 2023 19:10
@samopolacek samopolacek force-pushed the check_write_issues branch 2 times, most recently from 7b16fdb to b6282ef Compare July 26, 2023 19:34
@firewave
Copy link
Collaborator

We need to make sure we have tests for all these cases. That also needs to consider the issue addressed in danmar/simplecpp#339 i.e. fstream can successfully open directories but just not read from them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants