[PATCH] D95534: clang-cl: Invent a /winsysroot concept

[Nico Weber via Phabricator via cfe-commits]

thakis added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:75
+  for (llvm::sys::fs::directory_iterator DirIt(Directory, EC), DirEnd;
+       DirIt != DirEnd && !EC; DirIt.increment(EC)) {
+    if (!llvm::sys::fs::is_directory(DirIt->path()))
----------------
hans wrote:
> for being defensive, would it be safer to check !EC before comparing the iterators, in case DirIt is in a bad state because of an error?
Done (but this code is just moved up from below)


================
Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:88
+
+  return !Highest.empty();
+}
----------------
hans wrote:
> Since the success of the function is directly tied to whether the string is empty, maybe just return the string directly to simplify the API?
> 
> Oh I see, this comes from getWindows10SDKVersionFromPath(). Either way is fine I suppose.
Thanks, that's much nicer.


================
Comment at: clang/test/Driver/cl-sysroot.cpp:4
+
+// RUN: %clang_cl /winsysroot %t /c -- %t/foo.cpp
+// RUN: %clang_cl /vctoolsdir %t/VC/Tools/MSVC/14.26.28801 \
----------------
hans wrote:
> Driver/ tests don't usually run the actual compiler. Any reason not to just check the -### output?
I had that at first, but it felt a bit brittle to me. But sure, moved back to that.


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

https://reviews.llvm.org/D95534