<div dir="ltr">+ while (__first + __child < __last)<div><br></div><div>This is invalid when __first + __child > __last. Instead, use:</div><div><br></div><div> while (__child < __last - __first)<br><div class="gmail_extra">
<br></div><div class="gmail_extra">Likewise for later iterator additions.<br><br><br><div class="gmail_extra">+ if ((__first + __child + 1) < __last &&</div><div class="gmail_extra">+ __comp(*(__first + __child), *(__first + __child + 1)))</div>
<div class="gmail_extra">+ // right-child exists and is greater than left-child</div><div class="gmail_extra">+ ++__child;</div><div class="gmail_extra">+</div><div class="gmail_extra">+ // check if we are in heap-order</div>
<div class="gmail_extra">+ if (__comp(*(__first + __child), *(__first + __start)))</div><div class="gmail_extra">+ // we are, __start is larger than it's largest child</div><div class="gmail_extra">+ break;</div>
<div><br></div><div>This is slightly inefficient. Instead of computing left < right and then computing start < max(left, right), you can compute start < left and start < right, and save a comparison whenever start < left.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">+ // we are not in heap-order, swap the parent with it's largest child<br></div><div class="gmail_extra"><br></div><div class="gmail_extra">Typo "it's".</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">+ swap(*(__first + __child), *(__first + __start));<br></div><div class="gmail_extra"><br></div><div class="gmail_extra">This is inefficient: a swap is typically 3 moves, which is more than you need here. I think you should do what Howard's code did: rotate the loop so you first determine whether any elements need to move, and if they do, move the parent into a local variable, move each element over its parent instead of swapping, and drop the parent into the right spot at the end.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">+ // recompute the child based off of the updated parent<br></div><div class="gmail_extra"><br></div><div class="gmail_extra">"recompute the first child of the updated parent"?</div>
<div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">Having a __push_heap_back and a __sift_down seems weird to me. I think we should either have __sift_down and __sift_up or __push_heap_back and __push_heap_front (and I prefer the __sift names because we're not necessarily pushing at the front any more). Is there a reason to give these different names?</div>
<br><div class="gmail_quote">On Tue, Jul 15, 2014 at 12:18 PM, David Majnemer <span dir="ltr"><<a href="mailto:david.majnemer@gmail.com" target="_blank">david.majnemer@gmail.com</a>></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">
Ping.<br>
<div><div class="h5"><br>
On Wed, Jul 9, 2014 at 3:14 AM, David Majnemer <<a href="mailto:david.majnemer@gmail.com">david.majnemer@gmail.com</a>> wrote:<br>
> std::make_heap is currently implemented by iteratively applying a<br>
> sift_up-type algorithm.<br>
> Since sift-up is O(ln n), this gives std::make_heap a worst case time<br>
> complexity of O(n ln n). Since the C++ standard mandates that<br>
> std::make_heap make no more than O(3n) comparisons, this makes our<br>
> std::make_heap out of spec.<br>
><br>
> Fix this by introducing an implementation of __sift_down and switch<br>
> std::make_heap to create the heap using it. This gives std::make_heap<br>
> linear time complexity in the worst case.<br>
><br>
> This fixes PR20161.<br>
</div></div>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div></div>