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

Thu Jun 17 06:18:39 PDT 2021

hans added inline comments.

================
Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:1255
+    }
+  }
+
----------------
thakis wrote:
> `/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.
Hmm. I guess it depends on how people will use this flag, which isn't entirely clear. 

The way I think about it, users might use this to point to third-party libraries besides the system headers. For that reason it's different from the env var reads below, which are about finding the system headers.

In particular, I don't think /nostdlibinc (/X) should disable these flags, which is why I put it before that check.

I don't think people would combine this with /winsysroot, but if they want to, I'm not sure we should prevent it.

================
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"
----------------
thakis wrote:
> Should there be tests for the interaction with `/X`, `/winsysroot:`?
Yes, adding that.

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

https://reviews.llvm.org/D104387