[PATCH] This phabricator revision is the merge of 4 patches that aim to provide resolving of AT_abstract_origin and AT_specification attributes.

[Frédéric Riss]

> 
> ================
> Comment at: lib/DebugInfo/DWARFContext.cpp:401
> @@ +400,3 @@
> +                  const std::unique_ptr<UnitType> &RHS) const {
> +    return LHS->getNextUnitOffset() <= RHS->getNextUnitOffset();
> +  }
> ----------------
> I have a sneaking suspicion that <= ordering would violate the requirements of lower_bound... 
> 
> I /suspect/ the right answer is:
> 
> Off < RHS->getOffset()
> 
> LHS->getNextUnitOffset() <= Off
> 
> LHS->getNextUnitOffset() <= RHS->getOffset()
> 
> That should be a proper "less than" for a half open range [offset, next unit offset), I think... maybe?

I think you’re generally right. But all the documentation I can find about lower_bound tells me that only the comp(Unit, Offset) will be used by lower_bound. The other 2 functions make it adhere to the Compare concept, but I find it strange to implement unused functions just for that. Would it be legal to just pass a function accepting a Unit and an Offset?

BTW, is there an authoritative freely available source regarding these sort of STL usage details? 

> 
> On 05 Sep 2014, at 22:46, Alexey Samsonov <vonosmas at gmail.com> wrote:
> 
> IIRC, originally getCompileUnitForOffset() was supposed to work only for the exact Offsets (which are returned by DWARFUnit::getOffset()). Now that you're changing it to work for arbitrary offsets, I think it makes more sense to rewrite it as follows:

Then the comment of the function in the header was wrong. 

> if (CUs.empty()) return nullptr;
> auto It = std::upper_bound(CUs.begin(), CUs.end(), Offset, OffsetComparator());
> if (It == CUs.begin()) return nullptr;
> --It;
> return (It->getOffset <= Offset && Offset < It->getNextUnitOffset()) ? It->get() : nullptr;
> 
> and leave the comparator as is. I would rather not play with non-strict comparisons.

I could even keep using lower_bound without modifying the comparator, and when it returns CUs.end(), check --CUs.end() for the given offset. It would be a bit more efficient than the above I believe.

So should I use David’s variation, remove the seemingly unused comparison functions, don’t touch the comparator object and rewrite the function? What do you prefer?

Fred