[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.


================
Comment at: lib/Driver/Job.cpp:312
   SmallVector<const char*, 128> Argv;
+  auto Envp = Environment.size() > 1
+                ? const_cast<const char **>(Environment.data())
----------------
Can you add `assert(Environment.back() == nullptr);`?


================
Comment at: lib/Driver/ToolChains/MSVC.cpp:478
+      size_t EnvBlockLen = 0;
+      while (EnvBlockWide[EnvBlockLen] != '\0') {
+        ++EnvCount;
----------------
`L'\0'`


================
Comment at: lib/Driver/ToolChains/MSVC.cpp:487-488
+      if (!llvm::convertUTF16ToUTF8String(
+             llvm::ArrayRef<char>(reinterpret_cast<char *>(EnvBlockWide.get()),
+                                  EnvBlockLen * sizeof(EnvBlockWide[0])),
+             EnvBlock))
----------------
There's an overload of `convertUTF16ToUTF8String` that takes an `ArrayRef<UTF16>`.  So I think you can just write this:

```
if (!llvm::convertUTF16ToUTF8String(makeArrayRef(EnvBlockWide.get(), EnvBlockLen)))
```


================
Comment at: lib/Driver/ToolChains/MSVC.cpp:492
+
+      Environment.reserve(EnvCount);
+
----------------
`EnvCount+1`, for the extra null terminator.


================
Comment at: lib/Driver/ToolChains/MSVC.cpp:497
+      // find it.
+      for (const char *Cursor = EnvBlock.data(); *Cursor != '\0';) {
+        llvm::StringRef EnvVar(Cursor);
----------------
We could avoid the manual index and loop counters if we do something like this:

```
StringRef Cursor(EnvBlock);
while (!Cursor.empty()) {
  StringRef ThisVar;
  std::tie(ThisVar, Cursor) = Cursor.split('\0');
}
```




================
Comment at: lib/Driver/ToolChains/MSVC.cpp:499
+        llvm::StringRef EnvVar(Cursor);
+        if (EnvVar.startswith_lower("path=")) {
+          using SubDirectoryType = toolchains::MSVCToolChain::SubDirectoryType;
----------------
Too bad `StringRef` doesn't have `consume_front_lower()`.  That would have been perfect here.


================
Comment at: lib/Driver/ToolChains/MSVC.cpp:517
+      }
+    }
+  SkipSettingEnvironment:
----------------
I think you need to push 1 more null terminator onto the end here to terminate the block.


https://reviews.llvm.org/D30991