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

[Hans Wennborg via Phabricator via cfe-commits]

hans 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()))
----------------
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?


================
Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:88
+
+  return !Highest.empty();
+}
----------------
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.


================
Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:1111
 
   std::error_code EC;
   llvm::SmallString<128> IncludePath(SDKPath);
----------------
EC is unused now


================
Comment at: clang/test/Driver/cl-sysroot.cpp:2
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
----------------
fancy :)


================
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 \
----------------
Driver/ tests don't usually run the actual compiler. Any reason not to just check the -### output?


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

https://reviews.llvm.org/D95534