<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On 08 Sep 2014, at 19:45, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com" class="">vonosmas@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><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; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br class="Apple-interchange-newline">On Mon, Sep 8, 2014 at 2:39 AM, Frédéric Riss<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:friss@apple.com" target="_blank" class="">friss@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><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 class="">><br class="">> ================<br class="">> Comment at: lib/DebugInfo/DWARFContext.cpp:401<br class="">> @@ +400,3 @@<br class="">> + const std::unique_ptr<UnitType> &RHS) const {<br class="">> + return LHS->getNextUnitOffset() <= RHS->getNextUnitOffset();<br class="">> + }<br class="">> ----------------<br class="">> I have a sneaking suspicion that <= ordering would violate the requirements of lower_bound...<br class="">><br class="">> I /suspect/ the right answer is:<br class="">><br class="">> Off < RHS->getOffset()<br class="">><br class="">> LHS->getNextUnitOffset() <= Off<br class="">><br class="">> LHS->getNextUnitOffset() <= RHS->getOffset()<br class="">><br class="">> That should be a proper "less than" for a half open range [offset, next unit offset), I think... maybe?<br class=""><br class=""></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 class=""><br class="">BTW, is there an authoritative freely available source regarding these sort of STL usage details?<br class=""><span class=""><br class="">><br class="">> On 05 Sep 2014, at 22:46, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com" class="">vonosmas@gmail.com</a>> wrote:<br class="">><br class="">> 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 class=""><br class=""></span>Then the comment of the function in the header was wrong.<br class=""></blockquote><div class=""><br class=""></div><div class="">Yes, aparently it is wrong. Otherwise function implementation is plain wrong.</div><div class=""> </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 class=""><br class="">> if (CUs.empty()) return nullptr;<br class="">> auto It = std::upper_bound(CUs.begin(), CUs.end(), Offset, OffsetComparator());<br class="">> if (It == CUs.begin()) return nullptr;<br class="">> --It;<br class="">> return (It->getOffset <= Offset && Offset < It->getNextUnitOffset()) ? It->get() : nullptr;<br class="">><br class="">> and leave the comparator as is. I would rather not play with non-strict comparisons.<br class=""><br class=""></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 class=""></blockquote><div class=""><br class=""></div><div class="">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 class="">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 class="">strictly greater than the provided value, and decrement the result unconditionally.</div><div class=""><br class=""></div></div></div></blockquote><div><br class=""></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><br class=""></div><div>Fred</div><div><br class=""></div><div><br class=""></div><blockquote type="cite" class=""><div class=""><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; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 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 class=""><br class="">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; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><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; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class=""></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; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">--<span class="Apple-converted-space"> </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; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><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; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">Alexey Samsonov<br class=""><a href="mailto:vonosmas@gmail.com" target="_blank" class="">vonosmas@gmail.com</a></div></div></blockquote></div><br class=""></body></html>