[PATCH] D36201: Merge manifest namespaces.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:108-110
+  int HRef1Priority = std::distance(HRef1Position, std::end(MtNsHrefsPrefixes));
+  int HRef2Priority = std::distance(HRef2Position, std::end(MtNsHrefsPrefixes));
+  return HRef1Priority > HRef2Priority;
----------------
Since HRef1Position and HRef2Position are iterators for the same map, you can compare them directly, can't you?


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:130-134
+  for (auto &Ns : MtNsHrefsPrefixes) {
+    if (xmlStringsEqual(HRef, TO_XML_CHAR(Ns.first.data()))) {
+      return TO_XML_CHAR(Ns.second.data());
+    }
+  }
----------------
Sorry for picking up styles, but even in the same patch, you are using two styles. Here, you are doing

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

but in the above function you did

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

Consistency matters because it helps people who read your code, so please pay a bit more attention to consistency. It is also a good practice to read your code yourself several times after you think you "finish" your patch, as there's always something you can improve as a finishing touch. It will shorten the time for code review as (1) you will unlikely to get nitpicking comments on style and (2) consistent coding style makes code reviewers more confident that you know what you are doing.


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:144-150
+  xmlNsPtr Def = search(HRef, Node);
+  if (Def)
+    return Def;
+  Def = xmlNewNs(Node, HRef, getPrefixForHref(HRef));
+  if (!Def)
+    return make_error<WindowsManifestError>("failed to create new namespace");
+  return Def;
----------------
Maybe a better way of writing this is to use assignments in `if`s.

  if (xmlNsPtr Def = search(HRef, Node))
    return Def;
  if (xmlNsPtr  Def = xmlNewNs(Node, HRef, getPrefixForHref(HRef)))
    return Def;
  return make_error<WindowsManifestError>("failed to create new namespace");



================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:171
+                                       xmlNodePtr Node) {
+
+  if (Node == nullptr)
----------------
Remove this blank line.


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:217
+          return E;
+      } else if (namespaceOverrides(OriginalAttribute->ns->href,
+                                    Attribute->ns->href)) {
----------------
It's good to do `continue` here, as early continues are preferred.


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:234
+        // closest inherited default is the lower priority one.
+      } else if (Attribute->ns->prefix || OriginalAttribute->ns->prefix ||
+                 (ClosestDefault &&
----------------
Ditto


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


https://reviews.llvm.org/D36201