-
Notifications
You must be signed in to change notification settings - Fork 15.6k
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
Refine check for unordered_{map|set} availability. #766
Conversation
// Use C++11 unordered_{map|set} if available. | ||
#if ((__cplusplus >= 201103L) && \ | ||
((defined(__GLIBCXX__) && (__GLIBCXX__ > 20090421)) || \ | ||
(defined(_LIBCPP_VERSION) && (_LIBCPP_STD_VER >= 11)))) |
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.
The defined() checks are redundant, just compare the values, as they default to 0 if not defined.
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.
OK. Done.
Have you tested on Linux/Mac about whether this works for both libstdc++ and libc++? |
Chromium builds on Linux with libc++ (I believe with C++11 library support) and Mac with libstdc++ (definitely without C++11 library support), in both cases with C++11 language support enabled, and my original patch here built correctly on both platforms. I don't personally have Linux or Mac machines to try test-building various different configurations beyond that. |
For platforms that don't have the C++11 library support, would the rest of the logic figure out the correctly the hashmap/set to use? e.g. would the chrome Mac fall back to use tr1? |
If you lack C++11 library support it behaves just as today's check code does when you lack C++11 language support. So, yes, on Mac the next thing that happens is we try to fall back to tr1. Basically the behavior is identical, we're just refining the details of the check so that instead of "C++11 language?" we're asking "C++11 language and library?" |
LGTM |
It's not enough to check for C++11 language support, as it's possible for projects to enable C++11 language and library features independently (e.g. Chromium currently does this). Instead, explicitly check the library version to see if it is recent enough to include unordered_{map|set}.
Refine check for unordered_{map|set} availability.
…ing compatibility, also fixes protocolbuffers#766 and fixes 767
Refine check for unordered_{map|set} availability.
It's not enough to check for C++11 language support, as it's possible for
projects to enable C++11 language and library features independently (e.g.
Chromium currently does this). Instead, explicitly check the library version to
see if it is recent enough to include unordered_{map|set}.