On 27/06/2019 09:46, Louis Dionne wrote:
I find the customization point appropriate. I find that a lot of people's adversity for specialization and opening namespaces is not founded. My experience so far has been that ADL-based customization points, on the contrary, have more pitfalls and are harder to use. So I like the proposed design for customization points.
Perhaps I'm missing something, but as I read it the proposed design for customization is "specialize out_ptr itself". Which could alternatively be written as "reimplement the entire library yourself". So I'm not sure I agree that this could be called a "customization point"...
I found the implementation to be somewhat difficult to follow, especially for such a simple utility. There's a lot of indirection layers and it's unclear to me what's necessary and what's unjustified sophistication. For example, I would have tried not to have such a deep inheritance hierarchy:
class inout_ptr_t : public detail::core_inout_ptr_t /* this is either simple_inout_ptr_t or clever_inout_ptr_t */ class simple_inout_ptr_t : public base_inout_ptr_impl struct base_inout_ptr_impl : base_out_ptr_impl
Related: why does out_ptr inherit from std::tuple? Surely this should be a has-a relation, not an is-a relation? Granted this does allow for a zero-base-size optimization, which is useful as most of the time it would be an empty tuple, but it still feels wrong (and would allow inappropriate slicing if you try to pass an out_ptr as a tuple parameter, since it's public inheritance).
There's one thing I really dislike about the implementation, though. It's the clever aliasing trick. While it is clever, it's unsafe and it adds complexity to the library. In my opinion, this kind of thing should live merely in a separate experimental branch, or always be enabled in the cases where we know for sure it's safe (I think that's never but I'm not sure).
I tend to agree. While strict-aliasing rules are a bit of a pain in the butt, sneaking around behind them and poking private members of unique_ptr and friends doesn't seem like a good idea, even if it does improve benchmarks. And "UB-based optimization" does not inspire confidence.