Move jpeg_finish_compress out of destructor #433
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Moving the function out will ensure that no function that can error will be called inside a destructor, thus leaving no chance to get two exceptions in flight or finishing compression even if an error occured
|
I've lurked around and haven't spotted resource leak. Could anyone please confirm this? This commit should deal with both undefined behavior, termination and resource leakage if that's right. |
mloskot
approved these changes
Feb 14, 2020
There was a problem hiding this comment.
LGTM!
I think it is a good idea to add comment in ~writer_backend(), something like:
// JPEG compression object destruction does not signal errors,
// unlike jpeg_finish_compress called elsewhere,
// so there is no need for the setjmp bookmark here.
jpeg_destroy_compress(get());
This should help a suspicious reader to avoid an urge to patch it :)
Added comments to explain why finish compress was moved out of destructor, and did stylistical changes
|
@mloskot , thanks! I did as you mentioned, and I believe the |
mloskot
requested review from
stefanseefeld
and removed request for
stefanseefeld
|
Awesome. Thanks again @simmplecoder & @misos1 for your efforts. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This pull request removes the
jpeg_finish_compresscall from destructor, which will ensure no erroring function can be called when an exception is in flight or the destructor can fail. If we would throw out of destructor some time in the future, we will have to putnoexcept(false), as destructors arenoexceptby default.Thanks a lot to @misos1 for having productive discussion about the
setjmpandlongjmpin the original issue.References
#416
Tasklist