-
Notifications
You must be signed in to change notification settings - Fork 7.2k
Jpeg unpin - Cherrypicking #4288 #4344
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
Conversation
prabhat00155
left a comment
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.
Thanks @datumbox, a couple of minor comments.
| // For libjpeg version <= 9b, the out_size parameter in jpeg_mem_dest() is | ||
| // defined as unsigned long, where as in later version, it is defined as size_t. | ||
| // For windows backward compatibility, we define JpegSizeType as different types | ||
| // according to the libjpeg version used, in order to prevent compilcation |
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.
Typo in compilation.
| // according to the libjpeg version used, in order to prevent compilcation | ||
| // errors. | ||
| #if defined(_WIN32) || !defined(JPEG_LIB_VERSION_MAJOR) || \ | ||
| (JPEG_LIB_VERSION_MAJOR < 9) || \ |
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.
Why () around single condition?
|
Thanks for the quick review @prabhat00155. Definitely agree with your review comments. Note that this is a cherrypick from a different PR that landed on main branch that we try to bring on v0.10.1. Do you think it would be best to make this changes on a separate PR against main branch? |
Makes sense, I can do that, thanks! |
Fixes #4181 on 0.10