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