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

[Nico Weber via Phabricator via cfe-commits]

thakis accepted this revision.
thakis added a comment.
This revision is now accepted and ready to land.

That's a good argument. lgtm.



================
Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:1273
+
+    if (include_var) {
+      StringRef(*include_var)
----------------
Why not keep the definition of include_var in the if condition like it was on the rhs? (And do it for ext_include_var too)? They're only used in the respective if body, right? i.e. like so:

```
if (llvm::Optional<std::string> include_var =
        llvm::sys::Process::GetEnv("INCLUDE")) {
  StringRef(*include_var)
          .split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
}
if (llvm::Optional<std::string> ext_include_var =
        llvm::sys::Process::GetEnv("EXTERNAL_INCLUDE")) {
  StringRef(*ext_include_var)
          .split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
}
```

(Might even do `for (auto s : {"INCLUDE", "EXTERNAL_INCLUDE"}) ...` but that makes the FIXME harder to do later, so maybe don't do that part :) )


================
Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:1255
+    }
+  }
+
----------------
hans wrote:
> 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.
I don't know what people in general want, but _I_ definitely want a mode that makes the compiler not read any env vars :) But fair enough, this flag explicitly opts in to reading env vars, so if I don't want that I can just not pass this flag.


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

https://reviews.llvm.org/D104387