[PATCH] D36201: Merge manifest namespaces.

[Eric Beckmann via Phabricator via llvm-commits]

ecbeckmann added inline comments.


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:82-85
+  for (xmlNodePtr Child = Parent->children; Child; Child = Child->next)
     if (xmlStringsEqual(Child->name, ElementName)) {
       return Child;
     }
----------------
ruiu wrote:
> I think I wrote this before, but I believe we usually avoid this style
> 
>   for (...)
>     if (...) {
>     }
> 
> You want to either use this style
> 
>   for (...) {
>     if (...) {
>     }
>   }
> 
> or
> 
>   for (...)
>     if (...)
>       ....
> 
Ah okay....I think I've heard other variations of a similar principle, but not this exact style rule.  I'll change this everywhere.


================
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;
----------------
ruiu wrote:
> Since HRef1Position and HRef2Position are iterators for the same map, you can compare them directly, can't you?
the only way to directly compare iterators is using std::distance.  However, behavior is undefined if the 2nd iterator is not reachable by incrementing the first iterator, which will often be the case for our usage.  Therefore, the only way to correctly compare is to first find each one's absolute position in the map, which I do here.


================
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());
+    }
+  }
----------------
ruiu wrote:
> 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.
I understand, consistency is important.  I have been reading the code myself before sending it out for review, but I guess there are certain areas I need to focus on more.


https://reviews.llvm.org/D36201