[libc++] Add GCC workaround in std::char_traits<char>::length()

[philnik]

Summary

GCC currently does not allow __builtin_strlen() during constant evaluation. This PR adds a workaround in std::char_traits<char>::length()

[• Quuxplusone]

The code itself looks reasonable, but I would expect to see some related changes in libcxx/test/.
Surely this allows us to eliminate one or more UNSUPPORTED: gcc-11 somewhere in the test suite.
Maybe even eliminate constexpr_char_traits.h altogether?! (But probably not.)

Also, looks like you're missing a level of indentation on lines 343–348.

[Mordante]

libcxx/include/__string

342

Please add a comment why this is needed and link to the GCC bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70816
This makes it easier to test later whether it's fixed by GCC.

[philnik]

• rebase
• re-enable string.view.ops/copy.pass.cpp for gcc-11
• Indent correctly
• Add comment about GCC bug

[philnik]

• Added __ in front of i

[Mordante]

LGTM after the build passes.

libcxx/include/__string

347

[philnik]

Added char_type() around '\0'

[ldionne]

Thanks!

[• Quuxplusone]

I'm still surprised that there's only one test affected by this; but my git grep-fu was unable to turn up any others.

Btw, this is a great illustration of the mantra "If you aren't testing it, it doesn't work". With your original PR, with zero libcxx/test/ diffs, you didn't find that single dumb typo. (Not a knock on your abilities — everyone makes such typos!) With one touch of libcxx/test/, ta-da, buildkite detected the typo for you. :) We should always make sure that when we change code, we're changing a codepath that is actually tested in the test suite — if it's not tested, dollars to donuts it doesn't work.

Ship it!

[ldionne]

In D115795#3195968, @Quuxplusone wrote:

With one touch of libcxx/test/, ta-da, buildkite detected the typo for you.

I'm not sure I understand this -- are you implying that BuildKite doesn't trigger tests if libcxx/test isn't touched? If so, that would be incorrect -- BuildKite triggers whenever anything at all under libcxx/ is touched, and all the tests are run regardless of what's actually been touched.

[• Quuxplusone]

In D115795#3195991, @ldionne wrote:

In D115795#3195968, @Quuxplusone wrote:

With one touch of libcxx/test/, ta-da, buildkite detected the typo for you.

I'm not sure I understand this -- are you implying that BuildKite doesn't trigger tests if libcxx/test isn't touched?

Not at all. "If it's not being tested, then it doesn't work" is an early-2000s-Green-Hills-Software mantra, but it's more or less the same idea as "If you liked it then you should have put a test on it." But a little less emphasis on interpersonal motivations and more just Murphy's Law: If some feature could be broken without making the build red, then I can pretty much guarantee you that it is broken right now.

Thus, whenever there's a code change, there had better be a test change too: Either we're fixing a previously-untested codepath (as in this case) so we should add a test, or we're fixing a previously-wrong codepath so we should modify a test. (Or it's completely NFC, which also wasn't the case here.) In the original submission, there was a code change but zero test changes; I said "Code LGTM but I'd expect to see a test change too" (because of everything I just said). And then @philnik added that test change, and lo and behold, that change immediately identified a typo-bug that had slipped past (I'm assuming) both of us!
If (hypothetically, and maybe you think this'd never happen ;)) we had all LGTM'ed the code and shipped it, it would have totally broken constexpr string_view on GCC. "Constexpr string_view on GCC" is the thing in this instance that we weren't testing (because it was XFAIL'ed), and therefore didn't work... and therefore, even after we thought "yeah, this should fix it," it actually continued not-to-work... until we put a test on it. :)

None of this has any particular ramifications for this PR at all. I'm just pointing at it and saying "Look, perfect minimal gem of an illustration as to why code changes should imply test changes!" This was like an 8-line diff, "obviously" correct. Staring at the code didn't find the typo (at least, not in my case). Mindless (at least, in my case ;)) application of the mantra led to its being found.

[ldionne]

In D115795#3196141, @Quuxplusone wrote:

In D115795#3195991, @ldionne wrote:

In D115795#3195968, @Quuxplusone wrote:

With one touch of libcxx/test/, ta-da, buildkite detected the typo for you.

I'm not sure I understand this -- are you implying that BuildKite doesn't trigger tests if libcxx/test isn't touched?

Not at all. "If it's not being tested, then it doesn't work" is an early-2000s-Green-Hills-Software mantra, but it's more or less the same idea as "If you liked it then you should have put a test on it." But a little less emphasis on interpersonal motivations and more just Murphy's Law: If some feature could be broken without making the build red, then I can pretty much guarantee you that it is broken right now.

Thus, whenever there's a code change, there had better be a test change too: Either we're fixing a previously-untested codepath (as in this case) so we should add a test, or we're fixing a previously-wrong codepath so we should modify a test. (Or it's completely NFC, which also wasn't the case here.) In the original submission, there was a code change but zero test changes; I said "Code LGTM but I'd expect to see a test change too" (because of everything I just said). And then @philnik added that test change, and lo and behold, that change immediately identified a typo-bug that had slipped past (I'm assuming) both of us!
If (hypothetically, and maybe you think this'd never happen ;)) we had all LGTM'ed the code and shipped it, it would have totally broken constexpr string_view on GCC. "Constexpr string_view on GCC" is the thing in this instance that we weren't testing (because it was XFAIL'ed), and therefore didn't work... and therefore, even after we thought "yeah, this should fix it," it actually continued not-to-work... until we put a test on it. :)

None of this has any particular ramifications for this PR at all. I'm just pointing at it and saying "Look, perfect minimal gem of an illustration as to why code changes should imply test changes!" This was like an 8-line diff, "obviously" correct. Staring at the code didn't find the typo (at least, not in my case). Mindless (at least, in my case ;)) application of the mantra led to its being found.

What I had misunderstood is that the test kept failing due to a typo and was hence "passing" the XFAIL test, but for a different reason. I understand and agree with everything you've said now that I understand that. Thanks for clarifying though.

libcxx/include/__string

struct _LIBCPP_TEMPLATE_VIS char_traits<char>
     void assign(char_type& __c1, const char_type& __c2) _NOEXCEPT {__c1 = __c2;}
     static inline _LIBCPP_CONSTEXPR bool eq(char_type __c1, char_type __c2) _NOEXCEPT
             {return __c1 == __c2;}
     static inline _LIBCPP_CONSTEXPR bool lt(char_type __c1, char_type __c2) _NOEXCEPT
         {return (unsigned char)__c1 < (unsigned char)__c2;}
 
     static _LIBCPP_CONSTEXPR_AFTER_CXX14
     int compare(const char_type* __s1, const char_type* __s2, size_t __n) _NOEXCEPT;
-    static inline size_t _LIBCPP_CONSTEXPR_AFTER_CXX14
-    length(const char_type* __s)  _NOEXCEPT {return __builtin_strlen(__s);}
+
+    static inline size_t _LIBCPP_CONSTEXPR_AFTER_CXX14 length(const char_type* __s)  _NOEXCEPT {
+      // GCC currently does not support __builtin_strlen during constant evaluation.
+      // https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70816
+#ifdef _LIBCPP_COMPILER_GCC
+      if (__libcpp_is_constant_evaluated()) {
+        size_t __i = 0;
+        for (; __s[__i] != char_type('\0'); ++__i)
+            ;
+        return __i;
+      }
+#endif
+      return __builtin_strlen(__s);
+    }
+
     static _LIBCPP_CONSTEXPR_AFTER_CXX14
     const char_type* find(const char_type* __s, size_t __n, const char_type& __a) _NOEXCEPT;
     static inline _LIBCPP_CONSTEXPR_AFTER_CXX17
     char_type* move(char_type* __s1, const char_type* __s2, size_t __n) _NOEXCEPT
         {
             return __libcpp_is_constant_evaluated()
                        ? _VSTD::__move_constexpr(__s1, __s2, __n)
                        : __n == 0 ? __s1 : (char_type*)_VSTD::memmove(__s1, __s2, __n);

libcxx/test/std/strings/string.view/string.view.ops/copy.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-// GCC's __builtin_strlen isn't constexpr yet
-// XFAIL: gcc-11 && !(c++11 || c++14 || c++17)
-
 // <string_view>
 
 // size_type copy(charT* s, size_type n, size_type pos = 0) const;
 
 // Throws: out_of_range if pos > size().
 // Remarks: Let rlen be the smaller of n and size() - pos.
 // Requires: [s, s+rlen) is a valid range.
 // Effects: Equivalent to std::copy_n(begin() + pos, rlen, s).