-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Enhancement: libc++ provides string_view for all C++ language levels #2
Comments
@mclow @derekmauro @rogeeff Should be careful about |
@atkawa7 IIRC, the bug is about using libstdc++ with -std=<version older than c++17>. Since we are also checking __cplusplus >= 201703L, the bug should NOT be a concern. |
@atkawa7 The commit seems strange to me: the bug in the original code is in |
@zhangxy988 They are two different bugs but similar. So should be careful |
This is really about libc++, yes? So really we just want to check for libc++, and handle the msvc and libstdc++ cases separately. According to this stackoverflow we can do this by something like
@mclow is this a reasonable idea? |
Almost - you still have to check for the existence of the <string_view> header. Old versions of libc++ didn't have this. but if the header is there, then you have |
@mclow @JonathanDCohen Alternatively (if you don't have |
Maybe if you don't have |
Sorry for the silence here. I've had this sitting around internally for a bit, and there's some concern that libc++ backporting string_view is technically non-conforming, and could be surprising w.r.t. ADL. We also have to take a look at our FDO profiles before landing this as this would potentially change the mangled symbol names for anything taking a string_view |
I think that "non-conforming" is a bit harsh. It's an extension, like marking the default constructor of |
Sorry for the silence on this. I'm slightly wary about this because a couple reasons First, some of the people working on migrating google3 to c++17 have expressed some slight misgivings about it. Second, because, even though we don't really support it, this is potentially a break for use of abseil functions with ADL for a change which is, ideally otherwise invisible to the user and adds a corner case to the story of Abseil's handling of c++ versions. I agree with your sentiment that this is more an extension than some kind of non-conforming break, but seeing that string_view is a fairly simple type, I'm not sure the small risk is worth the payoff for this. But I'm definitely open to being convinced. |
After some more discussions on this, I'm going to close this issue. It's tempting but there are other internal migration factors which this seems to complicate, and the payoff for it is fairly low. Thank you for the idea, though :) Please feel free to re-open if you feel this is a poor decision. |
Those are pretty hard for me to comment on (not being internal).
I'm not going to contest. |
…t outside of generic programming (attempt #2 after internal fix) PiperOrigin-RevId: 529796927 Change-Id: I755b7d907f96f4a05d01620503bf0862ce35e847
…t outside of generic programming (attempt abseil#2 after internal fix) PiperOrigin-RevId: 529796927 Change-Id: I755b7d907f96f4a05d01620503bf0862ce35e847
…t outside of generic programming (attempt abseil#2 after internal fix) PiperOrigin-RevId: 529796927 Change-Id: I755b7d907f96f4a05d01620503bf0862ce35e847
…t outside of generic programming (attempt abseil#2 after internal fix) PiperOrigin-RevId: 529796927 Change-Id: I755b7d907f96f4a05d01620503bf0862ce35e847
abseil-cpp/absl/base/config.h
Line 363 in 2a62fbd
Yes, even C++03. :-)
You can test this thus:
The text was updated successfully, but these errors were encountered: