[PATCH] D35753: llvm-mt: implement simple merging of manifests, not factoring namespaces.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:99
+      XMLNodeImpl OriginalChildWithName;
+      if ((strcmp(reinterpret_cast<const char *>(Child->name), "text") != 0) &&
+          (strcmp(reinterpret_cast<const char *>(Child->name), "comment") !=
----------------
ecbeckmann wrote:
> ruiu wrote:
> > ruiu wrote:
> > > Do you need this reinterpret_cast? It looks like Child->name is of `xmlChar *` which is a typedef to `unsigned char *`.
> > Reverse the condition so that you can `continue` early.
> unfortunately const char * and const unsigned char *  are technically not related types, and must be casted back and forth to each other.
Is it done?


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:126
+#if LLVM_LIBXML2_ENABLED
+  for(auto& Doc : MergedDocs)
+    xmlFreeDoc(Doc);
----------------
ecbeckmann wrote:
> ruiu wrote:
> > Format
> where is format error here?
There wasn't a space between `for` and `(`. Now you have it. It is probably worth reviewing your patch yourself on Phabricator after you upload a new one, as I could easily find multiple trivial errors such as indentation errors, and it is easy to find errors on Phab rather than on an editor for some reason. :)


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:33
+                      "requestedPrivileges", "security", "trustInfo"}) {
+    if (S == StringRef(reinterpret_cast<const char *>(ElementName)))
+      return true;
----------------
I think you can remove explicit instantiation of StringRef.


https://reviews.llvm.org/D35753