[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/cmake/config-ix.cmake:604
+
+if (NOT LIBXML2_FOUND AND NOT LLVM_DISABLE_LIBXML2 AND NOT (CMAKE_SYSTEM_NAME MATCHES "Windows"))
+  find_package(LibXml2)
----------------
Does it make sense to not do this on Windows?


================
Comment at: llvm/include/llvm/Support/WindowsManifestMerger.h:32
+
+#if defined(LIBXML2_ENABLED)
+#include <libxml/xmlreader.h>
----------------
You can use `#ifdef`


================
Comment at: llvm/include/llvm/Support/WindowsManifestMerger.h:51
+  static char ID;
+  WindowsManifestError(Twine Msg);
+  void log(raw_ostream &OS) const override;
----------------
const Twine &Msg?


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:28
+Error WindowsManifestMerger::merge(MemoryBuffer &Manifest) {
+#if defined(LIBXML2_ENABLED)
+  xmlSetGenericErrorFunc((void *)this, WindowsManifestMerger::errorCallback);
----------------
Use `#ifdef` throughout this patch.


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:41
+
+std::unique_ptr<MemoryBuffer> WindowsManifestMerger::getMergedManifest() {
+  std::unique_ptr<MemoryBuffer> OutputBuffer;
----------------
This function returns something non-empty even if you dont' have libxml2, but I don't think that makes sense.  Just like above function which just returns `Error::success()`, I think it is better to return {} from this function if libxml is not available.


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:41
+
+std::unique_ptr<MemoryBuffer> WindowsManifestMerger::getMergedManifest() {
+  std::unique_ptr<MemoryBuffer> OutputBuffer;
----------------
ruiu wrote:
> This function returns something non-empty even if you dont' have libxml2, but I don't think that makes sense.  Just like above function which just returns `Error::success()`, I think it is better to return {} from this function if libxml is not available.
Looks like it is more straightforward to return an `std::vector<uint8_t>` from this function instead of a MemoryBuffer. MemoryBuffer is usually used to represent something you've read from files. In this case, this function just construct an in-memory data structure which can easily be represented as std::vector.


================
Comment at: llvm/test/CMakeLists.txt:61
           llvm-modextract
-          llvm-mt
           llvm-nm
----------------
Is this intentional?


================
Comment at: llvm/tools/llvm-mt/llvm-mt.cpp:68
 LLVM_ATTRIBUTE_NORETURN void reportError(Twine Msg) {
-  errs() << "llvm-mt error: " << Msg;
+  errs() << "llvm-mt error: " << Msg << ".\n";
   exit(1);
----------------
Remove a full stop.



================
Comment at: llvm/tools/llvm-mt/llvm-mt.cpp:77-79
+  if (!EC)
+    return;
+  reportError(EC.message());
----------------
nit: since the real function body is just one line, this is simpler

  if (EC)
    reportError


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


================
Comment at: llvm/tools/llvm-mt/llvm-mt.cpp:110
+      outs() << "Ignoring " << Arg->getOption().getName()
+             << " option; not supported.\n";
+    }
----------------
Remove a full stop and \n.

Error messages should start with lowercase letter.


================
Comment at: llvm/tools/llvm-mt/llvm-mt.cpp:122
   if (InputFiles.size() == 0) {
-    reportError("No input file specified.\n");
+    reportError("No input file specified");
   }
----------------
Ditto




================
Comment at: llvm/tools/llvm-mt/llvm-mt.cpp:129
     OutputFile = InputArgs.getLastArgValue(OPT_OUT);
+    errs() << OutputFile << "\n";
   } else if (InputFiles.size() == 1) {
----------------
Ditto


================
Comment at: llvm/tools/llvm-mt/llvm-mt.cpp:133
   } else {
-  	reportError("No output file specified.\n");
+    reportError("No output file specified");
   }
----------------
Ditto


================
Comment at: llvm/tools/llvm-mt/llvm-mt.cpp:149
+  if (OutputBuffer->getBufferSize() == 0) {
+    outs() << "Empty manifest not written.\n";
+  } else {
----------------
Ditto


https://reviews.llvm.org/D35425