[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:39
+
+XMLNodeImpl hasChildWithName(XMLNodeImpl Parent,
+                             const unsigned char *ElementName) {
----------------
has -> get


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:51
+
+const unsigned char *hasAttribute(XMLNodeImpl Node,
+                                  const unsigned char *AttributeName) {
----------------
Since this function returns not a boolean value but a value itself, `get` is a better prefix than `has`.


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:77-79
+      char *NameCopy = strdup(reinterpret_cast<const char *>(Attribute->name));
+      char *ContentCopy =
+          strdup(reinterpret_cast<const char *>(Attribute->children->content));
----------------
The memory you allocate using strdup needs to be freed somewhere using free(). Does your patch do that?


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:106
+              Twine("error when copying ") +
+              Twine(reinterpret_cast<const char *>(Child->name)));
+        if (NewChild->ns)
----------------
Remove Twine from the second expression


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:113
+              Twine("could not merge ") +
+              Twine(reinterpret_cast<const char *>(NewChild->name)));
+      } else if (auto E = treeMerge(OriginalChildWithName, Child))
----------------
I don't think you need an explicit instantiation of Twine for the second expression.


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:114
+              Twine(reinterpret_cast<const char *>(NewChild->name)));
+      } else if (auto E = treeMerge(OriginalChildWithName, Child))
+        return E;
----------------
Add {}.

Usual convention is, if one statement is a block (e.g. enclosed in {})) all statements in the same `if ~ else if ~ else ~` should be enclosed with {} as well. E.g. this is fine

  if (foo)
    bar;
  else
    baz;

and this is also fine

  if (foo) {
    bar
  } else if (bar) {
    baz
  }

but this isn't okay.

  if (foo) {
    bar
  } else
    baz


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:143-145
+  if (CombinedDoc == nullptr)
+    CombinedDoc = ManifestXML;
+  else {
----------------
Fix style


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:148-149
+    XMLNodeImpl AdditionalRoot = xmlDocGetRootElement(ManifestXML);
+    if (strcmp(reinterpret_cast<const char *>(CombinedRoot->name),
+               reinterpret_cast<const char *>(AdditionalRoot->name)) == 0 &&
+        isMergeableElement(AdditionalRoot->name)) {
----------------
I think we have too many occurrences of this pattern. Can you add a function that compares two xmlStrings?


https://reviews.llvm.org/D35753