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

Rui Ueyama via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Aug 10 11:21:43 PDT 2017

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 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.


More information about the llvm-commits mailing list