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

Zachary Turner via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 9 20:07:44 PDT 2017

zturner added inline comments.

Comment at: lld/COFF/DriverUtils.cpp:316
-// Create the default manifest file as a temporary file.
-TemporaryFile createDefaultXml() {
-  // Create a temporary file.
-  TemporaryFile File("defaultxml", "manifest");
-  // Open the temporary file for writing.
-  std::error_code EC;
-  raw_fd_ostream OS(File.Path, EC, sys::fs::F_Text);
-  if (EC)
-    fatal(EC, "failed to open " + File.Path);
+std::string createDefaultXml() {
+  std::string DefaultXml;
Can you mark this function `static`?

Comment at: lld/COFF/DriverUtils.cpp:352
+std::unique_ptr<MemoryBuffer> OutputBuffer;
Indentation is off here.  Is this clang-formatted?

Comment at: lld/COFF/DriverUtils.cpp:354
   // If manifest files are supplied by the user using /MANIFESTINPUT
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)
    return Merger.getMergedOutput();

and the decision about whether to use libxml or shell out to mt.exe would be part of the `WindowsManifestMerger` class.

Comment at: lld/COFF/DriverUtils.cpp:377
+  // Create the default manifest file as a temporary file.
+  TemporaryFile Default("defaultxml", "manifest");
+  std::error_code EC;
If we're not on Windows we should emit a warning that the manifest file was ignored since mt will not be available.


More information about the llvm-commits mailing list