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

[David Blaikie]

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

>
> On 08 Sep 2014, at 19:45, Alexey Samsonov <vonosmas at gmail.com> wrote:
>
>
> 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.
>
>
> You’re right of course. There is another solution I believe: use only
> strict comparisons against getNextUnitOffset in the comparator and use
> upper_bound as search function. This way, the returned unit is there first
> one where getNextUnitOffset is strictly greater than the searched offset
> which is exactly what we are looking for, isn’t it? David, what do you
> prefer?
>

I've lost track of which parts of this comparator we are reviewing where,
but anyway...

this discussion is about how to use lower_bound or upper_bound?

  1, 2, 3, 4[, end]

the lower_bound and upper_bound of:

3 is 3, 4
5 is end, end
1 is 1, 2

etc...

So why can't we just use lower_bound and check != end. If the range is
contiguous (which it is, right?) then that should suffice. Any non-end
result would be the element we're looking for.

(assuming the comparison provided is "CU->getNextOffset() <= Offset")

Or am I missing something?



>
> Fred
>
>
> 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/20140915/d22ae379/attachment.html>