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

[Hans Wennborg via Phabricator via cfe-commits]

hans added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:1273
+
+    if (include_var) {
+      StringRef(*include_var)
----------------
thakis wrote:
> 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 :) )
It's because split() creates StringRefs into include_var and ext_include_var, so they need to stay in scope until we're done with the Dirs object.

But that's pretty subtle, and the code doesn't look very nice. We're doing the same "get env var and split" dance three times now, so I'll move it to a helper lambda.


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

https://reviews.llvm.org/D104387