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

[Alexey Samsonov]

On Mon, Sep 8, 2014 at 2:39 AM, Frédéric Riss <friss at apple.com> wrote:

> >
> > ================
> > 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.
>

Yes, aparently it is wrong. Otherwise function implementation is plain
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.
>

I'm not sure I follow. std::lower_bound() returns an element which is
greater than or equal than the provided value. If you want to find the CU
with the largest offset, which is still less than the offset
you provide, you should always decrement the std::lower_bound(). But than
there is the special case for equal offset... that's why you should better
use upper_bound, which returns the element
strictly greater than the provided value, and decrement the result
unconditionally.


>
> 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




-- 
Alexey Samsonov
vonosmas at gmail.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140908/2c73f2f5/attachment.html>