[PATCH] Fix DWARFContext::getCompileUnitForOffset().

[Frederic Riss]

================
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();
----------------
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.

http://reviews.llvm.org/D5262