[PATCH] Fix DWARFContext::getCompileUnitForOffset().

[Alexey Samsonov]

================
Comment at: lib/DebugInfo/DWARFUnit.h:50
@@ -57,2 +49,3 @@
     bool operator()(uint32_t LHS,
                     const std::unique_ptr<UnitType> &RHS) const {
+      return LHS < RHS->getNextUnitOffset();
----------------
friss wrote:
> dblaikie wrote:
> > friss wrote:
> > > dblaikie wrote:
> > > > I'm rather surprised if this is the one function that's required. According to the spec, it looks like the specified value is passed as the RHS, not the LHS... 
> > > Note I'm using upper_bound bellow. The link you provided says:
> > > Change 25.3.3.2 [lib.upper.bound] paragraph 1 from:
> > > -1- Requires: Type T is LessThanComparable (lib.lessthancomparable).
> > > to:
> > > -1- Requires: The elements e of [first, last) are partitioned with respect to the expression !(value < e) or !comp(value, e)
> > > 
> > > So value is on the LHS and the element at the RHS.
> > Oh, right - thanks for the explanation.
> > 
> > In that case, could we write this comparison as "LHS < RHS->getOffset()". That would be the real less-than comparison of the offset.
> > 
> > But then I suppose you'd have to switch around to lower_bound? Would there be a problem with that? 
> > 
> > Sorry I'm being rather confused by all this... 
> > 
> > It's just given the definition of upper_bound, it's sort of weird to give a less than comparison that's subtly different (doesn't actually compare !< for the same unit) and then access the element returned - when the element should be one /past/ the equal elements you're looking for... except for the subtle comparison which ends up doing what you want.
> > 
> > So I suppose what I'm trying to suggest is:
> > 
> >   operator()(unique_ptr LHS, unsigned RHS)
> >     return LHS->getNextUnitOffset() <= RHS; // this is actually a "less than" comparison, because "next unit offset" is past the end of this unit, this could alternatively be written as "getNextUnitOffset() - 1 < RHS"
> >   }
> >   ...
> >   auto I = lower_bound... 
> >   ...
> > 
> > 
> > 
> > 
> For me 2 solutions work: 
> - upper_bound with Offset < RHS->getNextUnitOffset()
> - lower_bound with LHS->getNextUnitOffset() <= Offset 
> 
> upper_bound returns the first element comparing greater than Offset, thus the first Unit where the getNextUnitOffset is strictly greater than Offset. I find this definition quite logical.
> 
> lower_bound returns the first element 'not less than', i.e. the first element where the comparator returns false. Which is strictly equivalent, but IMHO not less than applied to the non-strict comparison requires a bit more thinking.
> 
> But I guess which one seems more logical depends on the mental model you have for both functions. I managed to convince myself that both are equivalent (I hope all this back and forth didn't confuse my logic :-) ), thus I really don't mind the version that gets committed.
FWIW I'm perfectly fine with the current implementation.

http://reviews.llvm.org/D5262