[PATCH] D36255: Integrate manifest merging library into LLD.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/COFF/DriverUtils.cpp:348
+static std::unique_ptr<MemoryBuffer>
+createManifestXmlWithInternalMt(std::string &DefaultXml) {
+  std::unique_ptr<MemoryBuffer> DefaultXmlCopy =
----------------
In general, we prefer StringRef over `std::string &` (or `const std::string &`). Can you replace?


================
Comment at: lld/COFF/DriverUtils.cpp:367
+static std::unique_ptr<MemoryBuffer>
+createManifestXmlWithExternalMt(std::string &DefaultXml) {
+  const Triple HostTriple(Triple::normalize(LLVM_HOST_TRIPLE));
----------------
Ditto


================
Comment at: lld/COFF/DriverUtils.cpp:368
+createManifestXmlWithExternalMt(std::string &DefaultXml) {
+  const Triple HostTriple(Triple::normalize(LLVM_HOST_TRIPLE));
+  if (!HostTriple.isOSWindows()) {
----------------
It is nice to explain why we don't want to execute `mt` command on non-Windows machines, because this is different from what we usually do (which is to try to execute a command and see if execution succeeded.)


================
Comment at: lld/COFF/DriverUtils.cpp:371
+    warn("manifest ignored because no manifest merging tool available");
+    return MemoryBuffer::getMemBufferCopy(std::string());
+  }
----------------
std::string() -> ""


================
Comment at: lld/test/lit.cfg:270
+# libxml2 to merge manifests.
+if (lit.util.which('cvtres', config.environment['PATH'])) or (config.llvm_libxml2_enabled == "1"):
+    config.available_features.add('manifest_tool')
----------------
ecbeckmann wrote:
> zturner wrote:
> > Can you line-break here?  This goes past 80.  Also, why are we checking for `cvtres` instead of `mt`?  The comment makes sense, but why not just check for `mt` directly?
> Checking for mt always seems to succeed, there are too many identically named tools I believe.  Note that this was the standard check for finding Microsoft utils even before I started making changes, you could search for rc and cvtres but not for mt.
Yeah, `mt` is a command to operate magnetic tapes, and even though most Unix machines don't have tape drives anymore, it is still distributed and installed as part of GNU commands by default. So checking existence of `cvtres` makes sense.


Repository:
  rL LLVM

https://reviews.llvm.org/D36255