[PATCH] D36201: Merge manifest namespaces.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: llvm/include/llvm/WindowsManifest/WindowsManifestMerger.h:49-51
+private:
+  class WindowsManifestMergerImpl;
+  std::unique_ptr<WindowsManifestMergerImpl> Impl;
----------------
Usually we write private members after public members. Consistency matter, so please make new code consistent with existing code in general.


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:79
+static xmlNodePtr getChildWithName(xmlNodePtr Parent,
                              const unsigned char *ElementName) {
+  for (xmlNodePtr Child = Parent->children; Child; Child = Child->next)
----------------
Please run clang-format every time you upload a new patch to phab.


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:268-275
+  if (!Node1 || !Node1->ns)
+    return Node2;
+  else if (!Node2 || !Node2->ns)
+    return Node1;
+  else if (namespaceOverrides(Node1->ns->href, Node2->ns->href))
+    return Node1;
+  else
----------------
No `else` after `return` (apply this rule to other places in this file.)


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:683
+
+std::unique_ptr<MemoryBuffer> WindowsManifestMerger::getMergedManifest() { return Impl->getMergedManifest(); }
+
----------------
clang-format.


https://reviews.llvm.org/D36201