[PATCH] D36201: Merge manifest namespaces.

[Eric Beckmann via Phabricator via llvm-commits]

ecbeckmann marked 4 inline comments as done.
ecbeckmann added a comment.

In https://reviews.llvm.org/D36201#830843, @zturner wrote:

> 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.


I agree changing to StringRef would be good for consistency.  However there are a couple reasons I see not to.  One is just simply the string type libxml2 uses is unsigned char *, so keeping as such whenever possible reduces the number of conversions.  Second, my merging implementation allows nullptr to be a valid string value, and is used to designate the prefix of a default xml namespace, and libxml also uses this representation.  Therefore we have things like making nullptr comparisons, which StringRef explicitly disallows.



================
Comment at: llvm/include/llvm/WindowsManifest/WindowsManifestMerger.h:49-51
+private:
+  class WindowsManifestMergerImpl;
+  std::unique_ptr<WindowsManifestMergerImpl> Impl;
----------------
ruiu wrote:
> Usually we write private members after public members. Consistency matter, so please make new code consistent with existing code in general.
Sorry, this was leftover from a previous version where I had inline forwarding calls in the header, so I needed the implementation class defined prior.


================
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)
----------------
ruiu wrote:
> Please run clang-format every time you upload a new patch to phab.
I had run clang-format before uploading, however I believe the problem is I was diffing against the wrong previous revision so not all my changes were covered.


================
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
----------------
ruiu wrote:
> No `else` after `return` (apply this rule to other places in this file.)
also changed 2 other places


https://reviews.llvm.org/D36201