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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
