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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision.
Quuxplusone 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.
----------------
mstorsjo wrote:
> 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.)
> the case of having MSYS2 available (with that package installed) but not using it for testing is maybe contrived

No, I agree; if that makes a difference, then it's important to mention!

I'm not thrilled by "you should add e.g. `-DLIBCXX_TARGET_TRIPLE=x86_64-windows-msvc` to the `cmake` command line", because again that feels like I'm being asked to do something vaguely unspecified — like, "Try this, but maybe secretly you should have tried something different, so if it doesn't work it's probably //your// fault not ours." ;)  Still, as I don't know what other option might work, I have no concrete suggestion for better wording. I'd say add the "e.g." if it improves the technical correctness, and then :shipit:


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97166