[PATCH] D104387: [clang-cl] Implement /external:I, /external:env, and EXTERNAL_INCLUDE support (PR36003)

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Nice, that's much less convoluted than I had feared :)



================
Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:1255
+    }
+  }
+
----------------
`/external:env` should happen after winsysroot I think. sysroots try to make builds hermetic and env vars defeat that.

I.e. this flag is more like the env var reads below than imsvc above – it reads the env mostly, instead of being a flag mostly.


================
Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:1266
+    llvm::Optional<std::string> include_var =
+        llvm::sys::Process::GetEnv("INCLUDE");
+    llvm::Optional<std::string> ext_include_var =
----------------
Maybe this should grow a FIXME to make regular INCLUDE not a system include at some point? Kind of sounds like this is the direction msvc is moving in with the existence of EXTERNAL_INCLUDE at least.


================
Comment at: clang/test/Driver/cl-include.c:10
 
-// RUN: env INCLUDE=/my/system/inc %clang_cl -### -- %s 2>&1 | FileCheck %s --check-prefix=STDINC
+// RUN: env INCLUDE=/my/system/inc env EXTERNAL_INCLUDE=/my/system/inc2 %clang_cl -### -- %s 2>&1 | FileCheck %s --check-prefix=STDINC
 // STDINC: "-internal-isystem" "/my/system/inc"
----------------
Should there be tests for the interaction with `/X`, `/winsysroot:`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104387