-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
…ark::DoNotOptimize` on both inputs and outputs and by removing the unnecessary and wrong `ABSL_RAW_CHECK` condition (`check != 0`) of `BM_ByteStringFromAscii_Fail` benchmark. Relevant comment: ``` // The DoNotOptimize(...) function can be used to prevent a value or // expression from being optimized away by the compiler. This function is // intended to add little to no overhead. // See: http://stackoverflow.com/questions/28287064 // // The specific guarantees of DoNotOptimize(x) are: // 1) x, and any data it transitively points to, will exist (in a register or // in memory) at the current point in the program. // 2) The optimizer will assume that DoNotOptimize(x) could mutate x or // anything it transitively points to (although it actually doesn't). // // To see this in action: // // void BM_multiply(benchmark::State& state) { // int a = 2; // int b = 4; // for (auto s : state) { // testing::DoNotOptimize(a); // testing::DoNotOptimize(b); // int c = a * b; // testing::DoNotOptimize(c); // } // } // BENCHMARK(BM_multiply); // // Guarantee (2) applied to 'a' and 'b' prevents the compiler lifting the // multiplication outside of the loop. Guarantee (1) applied to 'c' prevents the // compiler from optimizing away 'c' as dead code. ``` To see #1 and #2 in action, see: https://godbolt.org/z/ned1578ve PiperOrigin-RevId: 676588185 Change-Id: I7ed3e4bed8274b54ac7877316f2d82c33d68f00f
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: