[PATCH] D36201: Merge manifest namespaces.

[Zachary Turner via Phabricator via llvm-commits]

zturner added a comment.

Mostly style issues.  I'd like to see all the char pointers go away in favor of `StringRef`, unless there's a strong reason why this won't work.



================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:50-57
+static const std::pair<std::string, std::string>
+    MtNsHrefsPrefixes[] = {
+        {"urn:schemas-microsoft-com:asm.v1", "ms_asmv1"},
+        {"urn:schemas-microsoft-com:asm.v2", "ms_asmv2"},
+        {"urn:schemas-microsoft-com:asm.v3", "ms_asmv3"},
+        {"http://schemas.microsoft.com/SMI/2005/WindowsSettings",
+         "ms_windowsSettings"},
----------------
Why not have this be a vector of pairs of `StringRef`?  We're not modifying anything, and these will be in constant storage, so I think `StringRef` is fine?  Maybe even a `std::map<StringRef, StringRef>`, which can still use initializer list syntax.


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:68
 
-bool isMergeableElement(const unsigned char *ElementName) {
+static bool isMergeableElement(const unsigned char *ElementName) {
   for (StringRef S : {"application", "assembly", "assemblyIdentity",
----------------
It would be nice if we could deal entirely with `StringRef` and do conversions at the highest level possible so we never have to deal with this stuff.

```
StringRef toStringRef(const unsigned char *Str) {
  return StringRef(FROM_XML_CHAR(Str));
}

static bool isMergeableElement(StringRef S) {
  constexpr StringRef KnownList[] = { ... };
  return llvm::is_contained(KnownList, S);
}
```

then the above `xmlStringsEqual()` also just becomes `toStringRef(X) == toStringRef(Y)`, which would most likely just turn into `X == Y` because you'll be getting rid of the pointer-ness at the highest level and everything will actually be dealing with `StringRef` behind the scenes.


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:87
 
-const unsigned char *getAttribute(XMLNodeImpl Node,
-                                  const unsigned char *AttributeName) {
-#if LLVM_LIBXML2_ENABLED
+static xmlAttrPtr getAttribute(xmlNodePtr Node, const unsigned char *AttributeName) {
   for (xmlAttrPtr Attribute = Node->properties; Attribute != nullptr;
----------------
For a concrete example of the previous suggestion:

```
static xmlAttrPtr getAttribute(xmlNodePtr Node, StringRef AttributeName) {
  for (auto Attribute = Node->properties; Attribute; Attribute = Attribute->next) {
    if (toStringRef(Attribute->Name) == AttributeName)
      return true;
  }
  return false;
}
```


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:97-98
+// Check if namespace specified by HRef1 overrides that of HRef2.
+static bool namespaceOverrides(const unsigned char *HRef1,
+                        const unsigned char *HRef2) {
+  auto HRef1Position = std::find_if(
----------------
`namespaceOverrides(StringRef HRef1, StringRef HRef2)`


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:106-110
+  auto HRef2Position = std::find_if(
+      std::begin(MtNsHrefsPrefixes), std::end(MtNsHrefsPrefixes),
+      [HRef2](const std::pair<std::string, std::string> &element) {
+        return xmlStringsEqual(HRef2, TO_XML_CHAR(element.first.c_str()));
+      });
----------------
You can use the range-based `llvm::find_if(MtNsHrefsPrefixes, [HRef2]...);`  Alternatively, if my initial suggestion of making it a `map` works, you could just write: `auto HRef2Position = MtNsHrefsPrefixes.find(HRef2);`


https://reviews.llvm.org/D36201