[PATCH] D76997: Fix StringRef::strLen in windows with clang++ C++17

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added inline comments.


================
Comment at: llvm/include/llvm/ADT/StringRef.h:84
       return __builtin_strlen(Str);
+#elif __cplusplus > 201402L && !defined(_MSC_VER)
+      return std::char_traits<char>::length(Str);
----------------
isuruf wrote:
> isuruf wrote:
> > rnk wrote:
> > > aaron.ballman wrote:
> > > > 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.
> > > 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.
> > I'm using `_MSC_VER == 1911` and LLVM builds fine with that version. It doesn't have the correct constexpr value. Hence this differential.
> 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.
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76997/new/

https://reviews.llvm.org/D76997