[PATCH] D36201: Merge manifest namespaces.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Publishing comments about coding style first. I'll do real code review later.



================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:82-85
+  for (xmlNodePtr Child = Parent->children; Child; Child = Child->next)
     if (xmlStringsEqual(Child->name, ElementName)) {
       return Child;
     }
----------------
I think I wrote this before, but I believe we usually avoid this style

  for (...)
    if (...) {
    }

You want to either use this style

  for (...) {
    if (...) {
    }
  }

or

  for (...)
    if (...)
      ....



================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:273-275
+  if (Node->ns && Node->ns != getNamespaceWithPrefix(Node->ns->prefix, Node))
+    return true;
+  return false;
----------------
  if (foo)
    return true;
  return false;

is equivalent to

  return foo;


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:283-284
+    return true;
+  else
+    return false;
+}
----------------
`else` after `return`


https://reviews.llvm.org/D36201