Skip to content
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

Closed
mclow opened this issue Sep 26, 2017 · 14 comments
Closed

Enhancement: libc++ provides string_view for all C++ language levels #2

mclow opened this issue Sep 26, 2017 · 14 comments
Assignees

Comments

@mclow
Copy link

mclow commented Sep 26, 2017

#define ABSL_HAVE_STD_STRING_VIEW 1

Yes, even C++03. :-)

You can test this thus:

#ifdef __has_include
# if __has_include(<string_view>)
#  include <string_view>
#  if __cplusplus >= 201703L || defined(_LIBCPP_STRING_VIEW)
#   define ABSL_HAVE_STD_STRING_VIEW 1
#  endif
# endif
#endif
@atkawa7
Copy link

atkawa7 commented Sep 28, 2017

@zhangxy988
Copy link
Contributor

@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
Copy link

atkawa7 commented Sep 28, 2017

@zhangxy988
Copy link
Contributor

@atkawa7 The commit seems strange to me: the bug in the original code is in #if !defined(_MSC_VER) || __cplusplus >= 201703L which is ignoring the __cplusplus >= 201703L check on any non MSVC compiler (GCC, Clang), and this should be fixed as #if !defined(_MSC_VER) && __cpluscplus >= 201703L (or simply #if __cplusplus >= 201703L since _MSC_VER set __cpluscplus to 199711).
So, I don't think this has anything to do with the GCC bug.

@atkawa7
Copy link

atkawa7 commented Sep 28, 2017

@zhangxy988 They are two different bugs but similar. So should be careful

@JonathanDCohen
Copy link
Contributor

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

#include <ciso646>
// ...
#ifdef _LIBCPP_VERSION
#define ABSL_HAVE_STD_STRING_VIEW 1
#else 
// ...

@mclow is this a reasonable idea?

@mclow
Copy link
Author

mclow commented Oct 3, 2017

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 string_view

@EricWF
Copy link
Contributor

EricWF commented Oct 9, 2017

@mclow @JonathanDCohen Alternatively (if you don't have __has_include, but you should), you could probably check that _LIBCPP_VERSION >= 4000.

@mclow
Copy link
Author

mclow commented Oct 9, 2017

Maybe if you don't have __has_include maybe you should just fail (and say "no std::string_view)

@JonathanDCohen
Copy link
Contributor

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

@mclow
Copy link
Author

mclow commented Oct 23, 2017

I think that "non-conforming" is a bit harsh. It's an extension, like marking the default constructor of vector as noexcept.

@JonathanDCohen
Copy link
Contributor

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.

@JonathanDCohen
Copy link
Contributor

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.

@mclow
Copy link
Author

mclow commented Dec 4, 2017

but there are other internal migration factors which this seems to complicate, and the payoff for it is fairly low.

Those are pretty hard for me to comment on (not being internal).

Please feel free to re-open if you feel this is a poor decision.

I'm not going to contest.

copybara-service bot pushed a commit that referenced this issue May 5, 2023
…t outside of generic programming (attempt #2 after internal fix)

PiperOrigin-RevId: 529796927
Change-Id: I755b7d907f96f4a05d01620503bf0862ce35e847
netkex pushed a commit to netkex/abseil-cpp that referenced this issue Apr 3, 2024
…t outside of generic programming (attempt abseil#2 after internal fix)

PiperOrigin-RevId: 529796927
Change-Id: I755b7d907f96f4a05d01620503bf0862ce35e847
copybara-service bot pushed a commit that referenced this issue Sep 19, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants