[PATCH] D35425: Implement parsing and writing of a single xml manifest file.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: llvm/include/llvm/Support/WindowsManifestMerger.h:28
+#define LLVM_INCLUDE_LLVM_SUPPORT_WINDOWS_MANIFEST_MERGER_H
+#endif
+
----------------
This include guard is not correct. `#endif` should be at end of the file.


================
Comment at: llvm/include/llvm/Support/WindowsManifestMerger.h:60-61
+public:
+  WindowsManifestMerger() = default;
+
+  Error merge(const MemoryBuffer &Manifest);
----------------
Do you need this? Since you didn't define any other ctors, this is defined as a default ctor. Isn't it enough?


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:42-45
+  std::unique_ptr<MemoryBuffer> OutputBuffer;
+  unsigned char *XmlBuff;
+  int BufferSize = 0;
+#ifdef LIBXML2_ENABLED
----------------
I think you'll get a compiler warning if you compile it in an environment that doesn't have libxml, as BufferSize and XmlBuff will be unused.

A better way of doing it is to completely separate two conditions:

  std::unique_ptr<MemoryBuffer> WindowsManifestMerger::getMergedManifest() {
  #ifdef LIBXML2_ENABLED
    ...
    return OutputBuffer;
  #else
    return nullptr;
  #endif
  }



================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:68
+    return Error::success();
+  else
+    return make_error<WindowsManifestError>("invalid xml document");
----------------
Remove `else` after `return`.


================
Comment at: llvm/tools/llvm-mt/llvm-mt.cpp:78
+  if (EC)
+  reportError(EC.message());
+}
----------------
Indentation.


================
Comment at: llvm/tools/llvm-mt/llvm-mt.cpp:83
+  if (EC)
+  handleAllErrors(std::move(EC),
+                  [&](const ErrorInfoBase &EI) { reportError(EI.message()); });
----------------
Indentation.


https://reviews.llvm.org/D35425