P0288

[philnik777]

No description provided.

[Quuxplusone]

Awesome to see you working on this!

[Quuxplusone]

Nit: s/auto/size_t/
It'd be good to put a comment here (or in the commit message) explaining why you chose 6 as opposed to 5 or 4 or 20. Is there a specific concrete callable type that you're trying to fit into this buffer? Like, "a lambda that captures a std::string," or something like that?

[philnik777]

No, I just picked it basically at random. If you have some reasoning how large it should be I'm happy hear it. The main reasoning I've got is that it's the same size as function, but that's not much. I've actually made this a variable because I changed it a few times already (and didn't come to any conclusion what it should be).

[Quuxplusone]

I tried downloading the patch in its current state (because I thought I spotted a problem with const-qualified _Funcs like const auto lam = [](){}; move_only_function m = lam;), but it didn't pass even a simple smoke test for me; I wonder if I caught it in the middle of some changes.
Anyway, since our discussion, I decided to write up my suggestions about __call_ etc.; see what you think of Quuxplusone/P0288-move-only-function. [Untested code warning! but it passed my manual smoke tests]
That's not including my suggestion to combine __call_ and __destroy_ into a single __behaviors_ callback; on second glance I thought maybe it would be more important for performance that we can skip the call-through-a-function-pointer when __destroy_ == nullptr.

[Quuxplusone]

Don't forget to remove "unimplemented": True on the feature-test macro, too!

[philnik777]

I've chosen to remove the trivially relocatable optimization for now. I'll add it later, since it should be fully ABI compatible.

[Quuxplusone]

This doesn't seem right; unique_ptr<std::byte[]> will call delete [] p;, but I don't think that matches up with this arcane ::operator new call.

I strongly recommend eliminating all dependencies on std::byte. (That'll also eliminate a header dependency, which is nice.) Then, at least in my version I just used make_unique, but I'm curious if you see a reason that wouldn't work correctly here. (And if so, please write a regression test that catches it!)

[philnik777]

I don't think using make_unique would be incorrect here, but the explicit alignment allows operator new a bit more freedom in what it returns. Why exactly are you recommending to remove dependencies on std::byte?

[Quuxplusone]

Why exactly are you recommending to remove dependencies on std::byte?

Coincidentally, Vittorio Romeo just posted something about std::byte the other day: https://vittorioromeo.info/index/blog/debug_performance_cpp.html ( https://godbolt.org/z/Gs4ehe8rK ). His beef is the performance pessimization implied by byte's extra layers of indirection. But my own beefs are different from Vittorio's, and twofold:

• the compile-time penalty for dragging in the definition of std::byte (including a new header is slow, unnecessary dependencies trigger Hyrum's Law, each new dependency theoretically makes it one straw more difficult to avoid circular dependencies)
• the philosophical idea that "core-language is better than library": char > std::byte, struct > std::tuple, for > std::for_each, int[10] > std::array<int, 10>, alignof > std::alignment_of, alignas > std::aligned_storage, auto(x) > std::decay_t<X>(x), ::new > std::construct_at, etc. Basically, every time the core language gives a way to do a task without library dependencies, one should use the core-language way as a general rule. Use library workarounds only when the core language hasn't (yet) provided a built-in solution. (This can be justified by appeal to runtime debug performance, or minimizing dependencies, or greater readability, or whatever; but at some point it just becomes a law in its own right. I rarely need to peek under the hood of this law, these days.)

[Quuxplusone]

Throughout, reinterpret_cast is correct, but static_cast would be simpler. You can static_cast from void* to any-other-pointer-type, and for passing around these untyped memory addresses you should be using void* rather than std::byte* (per my strong recommendation above).

[Quuxplusone]

Seems like you should static_assert(__fits_in_buffer<_StoredFunc>) here. Or is that already done somewhere? I find this control flow hard to parse for some reason (although it's probably just GitHub's narrow column width).

Orthogonally, I also recommend eliminating the dependency on construct_at.

[Quuxplusone]

Zeroing out __func.__buffer_ doesn't seem necessary. This could be simple assignment.

[Quuxplusone]

(Bleah, I'm probably adding comments to the wrong GitHub entity, because it won't let me comment on line 247. Sorry.) Recommend eliminating the dependency on <array>.