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

[Zachary Turner via Phabricator via llvm-commits]

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 LLVM_LIBXML2_ENABLED
   // 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)
      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.


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


https://reviews.llvm.org/D36255