Fix StringRef::strLen in windows with clang++ C++17

[isuruf]

Summary

This was found when compiling f18 using clang++ on windows.
f18 requires C++17 standard, but char_traits::length is not
a constexpr as required by the C++17 standard.

[aaron.ballman]

I looked at the definition available in MSVC 2017 (15.9) and it does have constexpr support, but it requires setting the conformance mode to properly honor __cplusplus. If you set /Zc:__cplusplus, I believe this code should work. Have you given that a shot?

[isuruf]

I've only used this from clang. How do I set it using clang's gnu driver?

[aaron.ballman]

In D76997#2020466, @isuruf wrote:

I've only used this from clang. How do I set it using clang's gnu driver?

clang-cl.exe /std:c++17 /Zc:__cplusplus /E -Xclang -dM C:\Users\aballman\Desktop\test.cpp (test.cpp is an empty file) lists #define __cplusplus 201703L, while dropping either /Zc: or changing /std: changes the reported value.

[rnk]

If the compiler has the builtin, IMO we should prefer the builtin. It's always going to be better and faster than the library implementation. I would recommend reordering the ifdefs to prefer __builtin_strlen over the STL.

[isuruf]

Prefer compiler builtin over C++ standard library

[rnk]

lgtm

llvm/include/llvm/ADT/StringRef.h

84

Oh, I thought that would eliminate the need for this ifdef check. Do we actually support < 1916 MSVC versions? I guess we do, so this is necessary. :( Sorry that makes my original suggestion less helpful.

[aaron.ballman]

llvm/include/llvm/ADT/StringRef.h

84

I still think the addition of !defined(_MSC_VER) is incorrect and that LLVM's CMake needs to be updated to pass /Zc:__cplusplus to MSVC.

However, I think there's a different change to make possibly. We require MSVC 2017 with the latest updates installed (https://llvm.org/docs/GettingStartedVS.html#software), but _MSC_VER == 1916 is 2017 Update 9 (Visual Studio 2017 version 15.9.11). I currently have Visual Studio 2017 version 15.9.22 installed, so it seems like the version check for the builtin branch can turn into defined(_MSC_VER) and the changes to the STL branch can be removed.

[isuruf]

llvm/include/llvm/ADT/StringRef.h

84

I'm using _MSC_VER == 1911 and LLVM builds fine with that version. It doesn't have the correct constexpr value. Hence this differential.

[isuruf]

llvm/include/llvm/ADT/StringRef.h

84

I'm using a cross-compiling setup with clang-cl and MSVC headers and libraries. 1911 was the last version that Microsoft published as a package that I can use on Linux easily.

[aaron.ballman]

llvm/include/llvm/ADT/StringRef.h

84

I'm using _MSC_VER == 1911 and LLVM builds fine with that version. It doesn't have the correct constexpr value. Hence this differential.

This does not meet our minimum system requirements, unfortunately.

I'm using a cross-compiling setup with clang-cl and MSVC headers and libraries. 1911 was the last version that Microsoft published as a package that I can use on Linux easily.

This is an interesting use case to consider, thank you for sharing it! Does 19.11 support /Zc:__cplusplus? I have access to 19.10, which does not, and 19.14, which does. If 19.11 supports a conforming __cplusplus value, then I think supporting that version isn't much of a problem. If it doesn't support that flag, then we have to replicate this sort of workaround in too many places and I'd be opposed to supporting that version. For reference, 19.11 is VS 2017 Update 4 and 19.16 is VS 2017 Update 9, so there's a pretty big delta between the versions.

[isuruf]

llvm/include/llvm/ADT/StringRef.h

84

clang-cl supports that flag regardless of the VS installation used. I also see some codepaths on the MSVC headers that use #if __cplusplus > 201402. If there's any other info you'd like, I'd be happy to share.

[aaron.ballman]

llvm/include/llvm/ADT/StringRef.h

84

Thank you! I don't think I need any further information; I think we need to add the /Zc flag in CMake and remove the _MSC_VER changes here (but the reordering to prefer builtins is good). @rnk, does that make sense to you as well?

[isuruf]

llvm/include/llvm/ADT/StringRef.h

84

@aaron.ballman, that still doesn't fix my problem where std::char_traits<char>::length(Str) is not constexpr on 1911 even if __cplusplus is set to a higher value.

[mstorsjo]

llvm/include/llvm/ADT/StringRef.h

84

I'm curious - in which way was 1911 published that made it easier to use on Linux than other versions?

On https://github.com/mstorsjo/msvc-wine/ I have a python script that fetches Visual Studio installer manifests and downloads and unpacks the files without needing to actually run wine. (The other half of the repo, scripts for fixing up inconsistent header casing and wrapping MSVC in Wine, is optional if you just want it downloaded and unpacked.)

[aaron.ballman]

llvm/include/llvm/ADT/StringRef.h

84

Sorry, you're right, that's still true. I guess it boils down to whether we support 19.11 or not. According to our documentation, we don't support it, which is why I came to the conclusion I did and suggested those changes, but that doesn't help you much.

However, if we do want to support it with a small change, we could figure out what _MSC_VER value to test against and test that here. What I'm opposed to with this patch it restricts us from ever using the STL with Visual Studio, which is a heavy hammer.

[isuruf]

llvm/include/llvm/ADT/StringRef.h

84

What I'm opposed to with this patch it restricts us from ever using the STL with Visual Studio, which is a heavy hammer.

As @rnk said, it's always better to use the compiler builtins over STL.

[aaron.ballman]

llvm/include/llvm/ADT/StringRef.h

84

We seem to be going around in circles. This is what I'm looking for: if the builtin is available, use it (the builtin is available in all versions of MSVC we support). If the builtin is not available, check the STL and if it's new enough, use it (we should not get here for MSVC because we should be using the builtin). If the STL is not available, use the manual implementation (we shouldn't ever get here, but having a fallback seems sensible). I believe this is what I'm looking for:

    // Constexpr version of std::strlen.
    static constexpr size_t strLen(const char *Str) {
#if __has_builtin(__builtin_strlen) || defined(__GNUC__) || defined(_MSC_VER)
      return __builtin_strlen(Str);
#elif __cplusplus > 201402L
      return std::char_traits<char>::length(Str);
#else
      const char *Begin = Str;
      while (*Str != '\0')
        ++Str;
      return Str - Begin;
#endif
    }

We don't have to set /Zc in CMake to make this work, though I still argue we should be setting it regardless (though perhaps not as part of this patch).

llvm/include/llvm/ADT/StringRef.h

private:
     // by providing a specialized version
     static int compareMemory(const char *Lhs, const char *Rhs, size_t Length) {
       if (Length == 0) { return 0; }
       return ::memcmp(Lhs,Rhs,Length);
     }
 
     // Constexpr version of std::strlen.
     static constexpr size_t strLen(const char *Str) {
-#if __cplusplus > 201402L
-      return std::char_traits<char>::length(Str);
-#elif __has_builtin(__builtin_strlen) || defined(__GNUC__) || \
+#if __has_builtin(__builtin_strlen) || defined(__GNUC__) || \
     (defined(_MSC_VER) && _MSC_VER >= 1916)
       return __builtin_strlen(Str);
+#elif __cplusplus > 201402L && !defined(_MSC_VER)
+      return std::char_traits<char>::length(Str);
 #else
       const char *Begin = Str;
       while (*Str != '\0')
         ++Str;
       return Str - Begin;
 #endif
     }