<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 16, 2014 at 10:52 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Mon, Sep 15, 2014 at 11:22 PM, Frédéric Riss <span dir="ltr"><<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div><blockquote type="cite"><div>On 16 Sep 2014, at 02:33, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><br><br style="font-family:Menlo-Regular;font-size:11px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_quote" style="font-family:Menlo-Regular;font-size:11px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">On Mon, Sep 8, 2014 at 11:28 AM, Frédéric Riss<span> </span><span dir="ltr"><<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>></span><span> </span>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div><blockquote type="cite"><div>On 08 Sep 2014, at 19:45, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a>> wrote:</div><br><div><div class="gmail_quote" style="font-family:Menlo-Regular;font-size:11px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br>On Mon, Sep 8, 2014 at 2:39 AM, Frédéric Riss<span> </span><span dir="ltr"><<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>></span><span> </span>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>><br>> ================<br>> Comment at: lib/DebugInfo/DWARFContext.cpp:401<br>> @@ +400,3 @@<br>> +                  const std::unique_ptr<UnitType> &RHS) const {<br>> +    return LHS->getNextUnitOffset() <= RHS->getNextUnitOffset();<br>> +  }<br>> ----------------<br>> I have a sneaking suspicion that <= ordering would violate the requirements of lower_bound...<br>><br>> I /suspect/ the right answer is:<br>><br>> Off < RHS->getOffset()<br>><br>> LHS->getNextUnitOffset() <= Off<br>><br>> LHS->getNextUnitOffset() <= RHS->getOffset()<br>><br>> That should be a proper "less than" for a half open range [offset, next unit offset), I think... maybe?<br><br></span>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?<br><br>BTW, is there an authoritative freely available source regarding these sort of STL usage details?<br><span><br>><br>> On 05 Sep 2014, at 22:46, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a>> wrote:<br>><br>> 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:<br><br></span>Then the comment of the function in the header was wrong.<br></blockquote><div><br></div><div>Yes, aparently it is wrong. Otherwise function implementation is plain wrong.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span><br>> if (CUs.empty()) return nullptr;<br>> auto It = std::upper_bound(CUs.begin(), CUs.end(), Offset, OffsetComparator());<br>> if (It == CUs.begin()) return nullptr;<br>> --It;<br>> return (It->getOffset <= Offset && Offset < It->getNextUnitOffset()) ? It->get() : nullptr;<br>><br>> and leave the comparator as is. I would rather not play with non-strict comparisons.<br><br></span>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.<br></blockquote><div><br></div><div>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</div><div>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</div><div>strictly greater than the provided value, and decrement the result unconditionally.</div><div><br></div></div></div></blockquote><div><br></div></div></div><div>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?</div></div></div></blockquote><div><br></div><div>I've lost track of which parts of this comparator we are reviewing where, but anyway... <br><br>this discussion is about how to use lower_bound or upper_bound?<br><br>  1, 2, 3, 4[, end]<br><br>the lower_bound and upper_bound of:<br><br>3 is 3, 4<br>5 is end, end<br>1 is 1, 2<br><br>etc... <br><br>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.<br><br>(assuming the comparison provided is "CU->getNextOffset() <= Offset")<br><br>Or am I missing something?<br></div></div></div></blockquote><div><br></div></div></div><div>I didn’t update this patch, I was waiting for approval on D5262 which is the OffsetComparator subpart of this, to rebase it. But now I am confused… lower_bound with <= comparison was my first proposal here, but you were bothered by the non-strict comparison. </div></div></div></blockquote><div><br></div></div></div><div>Looking back over my comments, my source of confusion, as you say, came from the non-strictness (in the casual sense of the term, not necessarily the "strict/strict-weak ordering" sense which the standard uses - I actually don't understand enough about comparators to know when something is or is not a strict-weak ordering off-hand (probably one of those "if it feels right" for me things)).<br><br>Specifically it was the oddity of having all three operations using <= which didn't make sense. Indeed the suggested three versions I wrote includes what I'm now suggesting/considering (lower_bound + NextUnitOffset <= Offset), but without the other 2 so there's less to get confused by/worry about.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div>Now that we’ve gone through this a few times, I find the upper_bound (with Offset < NextUnitOffset) version more logical, </div></div></div></blockquote><div><br></div></span><div>I'd like to understand why you find it more logical - perhaps I'm missing something in my understanding.<br><br>I find the upper_bound version less clear because we provide a comparator that, in some sense, says that the unit that contains the Offset is actually "less than" the CU we're looking for... (at least that's how I think about it) and deliberately take the unit one beyond that element. Usually I would think of the result of upper_bound as "the thing greater than what I'm looking for" - I'd use it if I want to visit "everything after <blah>" but in this case it's actually the thing we're looking for - we've just muddled with the comparison.<br><br>Whereas using lower_bound with NextUnitOffset <= Offset seems to make sense to me - I'm searching for the element with "Offset" within its range. Any unit with a NextUnitOffset <= Offset is "less" than the unit I'm looking for.<br><br>I see Alexey was where this got into a discussion of upper_bound, strictness, etc.<br><br>Alexey - perhaps you can describe your preference?<br><br>As mentioned previously, you're more of an owner of this code than I am - so if you have a strong preference to some particular formulation, go with it. I'm just curious to understand.</div></div></div></div></blockquote><div><br></div><div>Ah, explaining preferences is hard. As Frederic mentioned, both ways are legit, depending on the mental model you choose. Parsing the code suggested is now somewhat straightforward to me.</div><div>I guess I just learned to ignore the names of std::lower_bound / std::upper_bound (they are nonsensical anyway) and instead substitute them with semantics. (upper_bound with a custom comparator,</div><div>is not "find a thing greater", it's just "find the leftmost element for which X holds").</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><span class="HOEnZb"><font color="#888888"><br><br>- David</font></span></div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div>but I think it is totally equivalent to lower_bound with (NextUnitOffset <= Offset). </div><div><br></div><div>Fred</div><span><br><blockquote type="cite"><div><div class="gmail_quote" style="font-family:Menlo-Regular;font-size:11px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><br></div><div>Fred</div><span><div><br></div><div><br></div><blockquote type="cite"><div><div class="gmail_quote" style="font-family:Menlo-Regular;font-size:11px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">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?<br><br>Fred</blockquote></div><br style="font-family:Menlo-Regular;font-size:11px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br clear="all" style="font-family:Menlo-Regular;font-size:11px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div style="font-family:Menlo-Regular;font-size:11px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br></div><span style="font-family:Menlo-Regular;font-size:11px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">--<span> </span></span><br style="font-family:Menlo-Regular;font-size:11px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div dir="ltr" style="font-family:Menlo-Regular;font-size:11px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">Alexey Samsonov<br><a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a></div></div></blockquote></span></div></div></blockquote></div></div></blockquote></span></div><br></div></blockquote></span></div><br></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div dir="ltr">Alexey Samsonov<br><a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a></div>
</div></div>