[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:317
+std::string createDefaultXml() {
+  std::string DefaultXml;
+  raw_string_ostream OS(DefaultXml);
----------------
Can you make it more lld-ish?

DefaultXml -> Ret


================
Comment at: lld/COFF/DriverUtils.cpp:343
   OS << "</assembly>\n";
-  OS.close();
-  return File;
-}
-
-static std::string readFile(StringRef Path) {
-  std::unique_ptr<MemoryBuffer> MB =
-      check(MemoryBuffer::getFile(Path), "could not open " + Path);
-  return MB->getBuffer();
+  OS.flush();
+  return DefaultXml;
----------------
I believe raw_string_ostream is unbuffered, so you don't need this.


================
Comment at: lld/COFF/DriverUtils.cpp:354
 
+#if LLVM_LIBXML2_ENABLED
   // If manifest files are supplied by the user using /MANIFESTINPUT
----------------
zturner wrote:
> I wonder if all this should be an implementation detail of `WindowsManifestMerger` itself.  Then this entire function would jsut become:
> 
> ```
>     WindowsManifestMerger Merger;
>     for (StringRef Filename : Config->ManifestInput)
>       Merger.mergeFromFile(Filename);
>     return Merger.getMergedOutput();
> ```
> 
> and the decision about whether to use libxml or shell out to mt.exe would be part of the `WindowsManifestMerger` class.
I agree with Zach. lld shouldn't know whether libxml is available or not. So, `#if LLVM_LIBXML2_ENABLED` in lld doesn't seem right.


https://reviews.llvm.org/D36255