-
-
Notifications
You must be signed in to change notification settings - Fork 434
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
Expose zlib constants for backwards compat #815
Conversation
84fd490
to
b8f1adf
Compare
Thanks for taking this on. |
Also I noticed anything even remotely modern for zlib uses |
Oh my, good catch.
I've now taken care of this in a2c1b2a |
8a930f9
to
a2c1b2a
Compare
Also regarding header guards.. unzip.h / zip.h ioapi.h |
Those aren't a big deal. And if we want to change it, it's not in the scope of this PR. |
#ifndef _ZLIB_H | ||
//#include "zlib.h" | ||
#if !defined(_ZLIB_H) && !defined(ZLIB_H) && !defined(ZLIB_H_) | ||
# if __has_include(<zlib-ng.h>) |
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.
I'm afraid __has_include
is not available in MSVC?
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.
Perhaps it will only work if included into C++ file?
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.
__has_include
is in VS 2017 15.3: https://learn.microsoft.com/en-us/cpp/overview/visual-cpp-language-conformance?view=msvc-170
Do we care about obsolete versions of Visual Studio which are more than 7 years old?
[edit]
Ah, maybe you meant that VS support is only for C++, as stated here:
https://stackoverflow.com/questions/71540280/make-has-include-portable-in-c
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.
.
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.
@nmoinvaz can we merge this PR as it is, and deal with VS separately?
Otherwise, I guess, all what is needed is to add some nearly empty ioapi.cc file which just contains include "ioapi.h"
? But I'm worried it could affect other platforms which don't expect C++ code.
Since compat files moved to compat dir, not sure how to get cmake to use those when installing. |
Fix #747
Supersedes #750 (cc @brad0)
I re-did the PR to account for the recent refactor in 7df56f7