[libcxx-commits] [PATCH] D92190: [libc++] fix std::sort(T**, T**)

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a subscriber: howard.hinnant.
ldionne added a comment.

In D92190#2435062 <https://reviews.llvm.org/D92190#2435062>, @Quuxplusone wrote:

> Aw, I wish I had seen this to bikeshed over it — it's relevant to my interests! (Finishing P0879 constexpr algorithms which includes `sort`; the nightmare minefield that is comparing pointers https://quuxplusone.github.io/blog/2019/01/20/std-less-nightmare/)
>
> - Casting `T**` to `any-kind-of-int*` isn't something you can do constexprly, is it?

No, since it's a `reinterpret_cast` in essence.

> - Does this `sort` overload exist //purely// to avoid instantiating `std::less<T*>`? why is that so important? (`git blame` offers no help; this codepath dates back to Howard's initial import.)

My hunch is that it's a manual attempt to perform outlining, i.e. to avoid instantiating the same `std::sort` code over and over again for different pointer types when they are performing essentially the same task on objects that have the same representation. I could be wrong, I'm just guessing.

> - Could this `sort` overload simply delegate to `_VSTD::sort(first, last, __less<void*>())`, to avoid the constexpr-unfriendly part? (That is, don't type-pun the actual elements; force an implicit conversion to `void*` before each comparison.)

If that results in worse generated code, then we'll have a difficult tradeoff to make. But if not, then I agree we could do that. Or we could drop the outlining attempt altogether. It all depends on whether writing `sort` that way actually provides a benefit with modern compilers or not, I think. We'd have to check.

Pinging @howard.hinnant in case he remembers what was the reason for writing `sort` that way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92190/new/

https://reviews.llvm.org/D92190