Release resources in error paths via cleanup#4402
Conversation
Replace direct returns in error-handling branches with a unified cleanup block that frees allocated resources before returning, improving code quality and robustness.
|
Unfortunately, merge conflicts have emerged since the review. |
| @@ -0,0 +1,132 @@ | |||
| /* | |||
There was a problem hiding this comment.
This file simply shouldn't exist anymore.
It's a duplicate.
There was a problem hiding this comment.
I tried to squash these three commits locally, but it seems that squashing a merge commit is not straightforward.
There was a problem hiding this comment.
I tried to squash these three commits locally, but it seems that squashing a merge commit is not straightforward.
This can be done at merge time, via the github UI.
| int _exit_code = 1; | ||
| (void)argc; | ||
| (void)argv; | ||
| int _exit_code = 0; |
There was a problem hiding this comment.
int _exit_code = 1;
(...)
int _exit_code = 0;
Declaring the same variable twice ?
This feels like it shouldn't work ?
I'm surprised it even compiles.
Even if it does, it looks to me that this is not the wanted behavior: due to overloading, _exit_code value would always be 0.
There was a problem hiding this comment.
Oh, thanks for pointing that out. This was duplicated during the merge—I’ll remove the second declaration.
There was a problem hiding this comment.
That being said, I still don't understand why CI did not turn red.
I would have expected this unit to fail compilation. But CI is green.
Which leaves use with a few options:
- It can actually compile and is legal (which I doubted, so that would be new learning for me)
- It fails, but the CI does not detect it and turns green anyway (bad)
- The file is never compiled during CI (bad, should be fixed)
There was a problem hiding this comment.
tested locally:
largeDictionary.c:76:9: error: redefinition of '_exit_code'
76 | int _exit_code = 0;
| ^
largeDictionary.c:73:9: note: previous definition is here
73 | int _exit_code = 1;
| ^
largeDictionary.c:76:9: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement]
76 | int _exit_code = 0;
| ^
Compilation fails as expected.
So we are left with the other 2 options.
There was a problem hiding this comment.
OK, I think it's a case of test not run in CI.
It seems to be a long test btw, maybe that's why it was not set in CI.
I think that a test that is not run in CI, and is just expected to be run manually from time to time,
loses a lot of its value.
If it's too long to run in CI, maybe it's worth making it shorter, or at least allow a shorter version to be run in CI.
This is somewhat separate from your PR, so you are not expected to act on this.
But it's also somewhat related too, since your changes in this PR are not captured by CI for the time being (hence errors remain invisible).
|
In #4416, I'm adding a test to CI, that should run this unit. It is expected to fail, due to an existing warning. It will be rebased on top of this PR once it's merged, |
4 participants
Replace direct returns in error-handling branches with a unified cleanup block that frees allocated resources before returning, improving code quality and robustness.