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

[Eric Beckmann via Phabricator via llvm-commits]

ecbeckmann 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)
----------------
ruiu wrote:
> Does it make sense to not do this on Windows?
Yes, apparently the version of libxml2 on Windows uses different headers than what we expect and linking it in would cause a problem.  Besides, if we are on windows we can just continue to shell out to the original mt.exe tool.


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:41
+
+std::unique_ptr<MemoryBuffer> WindowsManifestMerger::getMergedManifest() {
+  std::unique_ptr<MemoryBuffer> OutputBuffer;
----------------
ruiu wrote:
> 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.
In this case I feel it is more convenient to use MemoryBuffer because it directly takes ownership of the underlying data allocated by xmlDocDumpMemory().  For using a vector we would need an additional copy, and then explicitly delete the allocated memory.  Though perhaps this is okay for clarity?


================
Comment at: llvm/test/CMakeLists.txt:61
           llvm-modextract
-          llvm-mt
           llvm-nm
----------------
ruiu wrote:
> Is this intentional?
Yes down below I only add the test depends if libxml2 is found.


================
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);
----------------
ruiu wrote:
> Remove a full stop.
> 
When I rebase the previous patch these mistakes will go away.


https://reviews.llvm.org/D35425