[PATCH] D36201: Merge manifest namespaces.

[Eric Beckmann via Phabricator via llvm-commits]

ecbeckmann 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;
----------------
zturner wrote:
> ecbeckmann wrote:
> > zturner wrote:
> > > ecbeckmann wrote:
> > > > zturner wrote:
> > > > > ecbeckmann wrote:
> > > > > > zturner wrote:
> > > > > > > ecbeckmann wrote:
> > > > > > > > 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.
> > > > > > > Maps are ordered, so why not just use the ordering function directly?
> > > > > > > 
> > > > > > > ```
> > > > > > > return FROM_XML_CHAR(HRef1) > FROM_XML_CHAR(HRef2)?
> > > > > > > ```
> > > > > > Unfortunately HRef1 and HRef2 do not point within the table, but rather xmlChar arrays allocated by libxml2's parser, so we can't compare those.
> > > > > I think I'm still mis-understanding something.  
> > > > > 
> > > > > The code here is looking both entries up by key, then after determining that both keys are present, it then computes each iterator's distance from the end.  And if the first one is farther from the end (e.g. closer to the beginning) than the second one, it returns true, otherwise it returns false.
> > > > > 
> > > > > But when you iterate a map, it iterates in key order.  So saying "the first one is closer to the beginning" is identical to saying "the first one is less than the second one".  So as long as you use the same comparison function that the map uses internally to order keys, then it should be the same.  And in this case, the map seems to be using a string comparison.
> > > > > 
> > > > > So it seems like all you have to do is string compare the two arguments and return true if the first one is less than the second one?
> > > > I'm no longer using a map, but rather a vector of pairs, therefore the keys stay in the same order as they were put in the definition.  No comparison operator is used on the StringRefs.  We purely want to find the index in the vector and compare them.  So using StringRef / string comparison will not work here.
> > > Ahh, I see.  I didn't realize we were no longer using a map.  In that case, why doesn't `return Iter1 < Iter2;` work?  `vector::iterator` is a random accessor iterator, which [[ http://www.cplusplus.com/reference/iterator/RandomAccessIterator/ | supports relational operators ]].
> > find() in this case returns type "std::_Rb_tree_const_iterator" which does not support comparison.  I think it is because it is a vector of pairs, so this iterator is used?  I feel like a complicated template specification is required to get it to return vector::iterator
> Something doesn't seem right.  If it's `std::_Rb_tree_const_iterator`, then this can't possibly be a vector of pairs, it's a `std::map<StringRef, StringRef>`.
Ah shoot my apologies, I was looking at the wrong revision.  It indeed uses a map.  Actually, this seems to be a bug I inadvertantly created, because I assume the sort will put it in alphabetical order, when really it should stay in the same order.  I will change to a vector and use position comparison on iterators.


https://reviews.llvm.org/D36201