[libcxx-commits] [PATCH] D97166: [libcxx] [docs] Update docs about how to build for Windows

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/docs/BuildingLibcxx.rst:129-133
+Note that if running in an MSYS2 shell and the MSYS2 provided clang package
+is installed, that one ends up picked up as tests invoke plain `clang++`
+instead of `clang-cl`. As the MSYS2 provided `clang++` binary defaults to
+a non-MSVC target, add `-DLIBCXX_TARGET_TRIPLE=x86_64-windows-msvc` to the
+cmake configuration in this case.
----------------
Quuxplusone wrote:
> mstorsjo wrote:
> > Quuxplusone wrote:
> > > 
> > Thanks for the rewording.
> > 
> > One tweak though; adding that `LIBCXX_TARGET_TRIPLE` doesn't make it call `clang-cl` instead of `clang++`, but it adds `-target x86_64-windows-msvc` to the `clang++` command line.
> > 
> > What about "... you should tell `check-cxx` to make `clang++` use the right target by adding `-DLIBCXX_TARGET_TRIPLE=x86_64-windows-msvc` to the `cmake` command line above."?
> > 
> Ah, I see. I'm not thrilled by the structure "You should tell X to do Y by Z'ing" because it's ambiguous whether //my// Z'ing will make X do Y, or whether I'm supposed to do something unspecified in order to make X do-Y-by-Z'ing. Therefore I suggest the following re-rewrite:
> 
> If you have installed the MSYS2-provided clang package (which defaults
> to a non-MSVC target), you should add `-DLIBCXX_TARGET_TRIPLE=x86_64-windows-msvc`
> to the `cmake` command line above. This will instruct `check-cxx` to use
> the right target triple when invoking `clang++`.
Sounds good to me, but I'd like two minor tweaks: "If you are running in an MSYS2 shell and you have installed the MSYS2-provided clang package" - the case of having MSYS2 available (with that package installed) but not using it for testing is maybe contrieved, but I'd prefer clarity.

Secondly, I'd add an "e.g." before the cmake option, as the architecture in the triple merely is a probable suggestion. (I see I didn't have that in earlier versions either, but looking at it now, I'd like to add it.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97166