[PATCH] D36201: Merge manifest namespaces.

[Eric Beckmann via Phabricator via llvm-commits]

ecbeckmann added inline comments.


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:217
+          return E;
+      } else if (namespaceOverrides(OriginalAttribute->ns->href,
+                                    Attribute->ns->href)) {
----------------
ruiu wrote:
> ruiu wrote:
> > It's good to do `continue` here, as early continues are preferred.
> It's not done.
are you referring to the if block or the else if block?  Because on this comment's revision it refers to the else if and I have added a continue since then.


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:246
+      // namespace that doesn't use the attribute.
     } else {
+      xmlAttrPtr NewProp = xmlNewProp(OriginalNode, Attribute->name,
----------------
ruiu wrote:
> ruiu wrote:
> > Ditto
> Not done.
same


https://reviews.llvm.org/D36201