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

Mon May 18 23:57:41 PDT 2020

isuruf marked an inline comment as done.
isuruf 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);
----------------
aaron.ballman wrote:
> mstorsjo wrote:
> > isuruf wrote:
> > > aaron.ballman wrote:
> > > > isuruf wrote:
> > > > > aaron.ballman wrote:
> > > > > > 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.
> > > > > `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.
> > > > 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?
> > > @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.
> > 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.)
> 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.
> 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76997