[PATCH] D36201: Merge manifest namespaces.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


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


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


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:103
+// Check if namespace specified by HRef1 overrides that of HRef2.
+static bool namespaceOverrides(const unsigned char *HRef1,
+                               const unsigned char *HRef2) {
----------------
Overall, you can simplify this function as

  auto It1 = llvm::find_if(
      MtNsHrefsPrefixes,
      [=](const std::pair<StringRef, StringRef> &element) {
        return xmlStringsEqual(HRef1, TO_XML_CHAR(element.first.data()));
      });
  auto It2 = llvm::find_if(
      MtNsHrefsPrefixes,
      [=](const std::pair<StringRef, StringRef> &element) {
        return xmlStringsEqual(HRef2, TO_XML_CHAR(element.first.data()));
      });
  return It1 < It2;


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:107
+      MtNsHrefsPrefixes,
+      [HRef1](const std::pair<StringRef, StringRef> &element) {
+        return xmlStringsEqual(HRef1, TO_XML_CHAR(element.first.data()));
----------------
Coding style issue: element -> Element


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:114
+      MtNsHrefsPrefixes,
+      [HRef2](const std::pair<StringRef, StringRef> &element) {
+        return xmlStringsEqual(HRef2, TO_XML_CHAR(element.first.data()));
----------------
Instead of explicitly capturing variables, use `[=]` or `[&]`.

element -> Element


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:117-119
+  if (HRef2Position == std::end(MtNsHrefsPrefixes))
+    return true;
+  return HRef1Position < HRef2Position;
----------------
I believe you can just remove

  if (HRef2Position == std::end(MtNsHrefsPrefixes))
    return true;

because if `HRef2Position` is `std::end(MtNsHrefsPrefixes)`, expression `HRef1Position < HRef2Position` is always true in this context.


https://reviews.llvm.org/D36201