hamzasood marked an inline comment as done.
hamzasood added inline comments.


================
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))
----------------
zturner wrote:
> 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)))
> ```
Using that overload would involve casting wchar_t* to UTF16* (i.e. unsigned 
char*), which I think breaks aliasing rules.


================
Comment at: lib/Driver/ToolChains/MSVC.cpp:517
+      }
+    }
+  SkipSettingEnvironment:
----------------
zturner wrote:
> zturner wrote:
> > I think you need to push 1 more null terminator onto the end here to 
> > terminate the block.
> Actually if you use the `std::tie()` algorithm I proposed, then it will enter 
> the body of the loop on the terminating null and push it back (as long as 
> `MakeArgString` returns `nullptr` when its argument is empty, which I haven't 
> checked)
Command::setEnvironment adds a nullptr onto the end of any vector it's given. I 
figured it's simpler to do it there than to rely on the caller. Do you think 
that should be changed?


https://reviews.llvm.org/D30991



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to