[vlc-devel] [PATCH] vlc_common: Add C++ memory management helpers

[Hugo Beauzée-Luyssen]

Hey Filip,

On Fri, Jul 13, 2018, at 7:28 AM, Filip Roséen wrote:
> Hi again,
> 
> I now realize that the email I sent earlier suffers from a few
> unfortunate typos, but not only that; what I wrote about your helpers
> (accepting a custom deleter) is extremely vague.
> 
> Sorry about that. I hope the added comments in this email will make it
> easier to understand what I meant and mean.
> 
> Best Regards,\
> F
> 
> On 2018-07-12 23:17, Filip Roséen wrote:
> 
> > Helpers are always nice, but one immediate reaction to the patch is
> > that these four, as a consequence will force modules written in `C++`
> > to be compiled as at least `C++11`, something we formerly did not
> > demand.
> 
> The above was meant to be read as *"Helpers are nice, but an
> unfortunate consequence of putting them in `vlc_common.h` is that the
> C++ compiler used to build a module then must support (and enable)
> *c++11*, even if the implementation of said module does not mandate
> it."*
> 

We already require C11 support from the headers, so it seems fair to me to require C++11 as well :)
That being said, since the code will be moved to a difference helper to be included if needed, the requirement will go away

> > This could be circumvented by introducing `#if __cplusplus >= 201103`
> > so that we do not force an error diagnostic down someones throat who
> > are for whatever stuck with an older compiler. One could argue that we
> 
> *"Who are, for whatever **reason**, stuck..."*
> 
> > As far as I am aware, both of the above overloads with custom
> > releasers are doing more than they are required in terms of
> > `noexcept`. If `Releaser` in `std::unique_ptr<class T, class Releaser>`
> > is not an *lvalue-reference*, it shall not throw an exception in its
> > *move-* and/or *copy-constructor* (which one depends on the reference
> > type of `r`).
> 

Granted, though I felt it would be more readable to have the entire construction as the noexcept expression. Now I also agree that it feels super weird to have the same expression twice.

> The usage of *"shall"* in the above can be very misleading, at least I
> fear it might. I should have explained that constructing a
> `std::unique_ptr<T, U>( expr1, expr2 )` where `U` is not an *lvalue*
> type is *UB* if;
> 
>  - the result of *expr2* is an *lvalue*, and the *copy-constructor* of
>    `U` is not *nothrow copy-constructible*, or;
>  - the result of *expr2* is an *rvalue*, and the *move-constructor* of
>    `U` is not *nothrow move-constructible*.
> 
> What the above boils down to is that the expressions passed to the
> `noexcept` specifier of the two functions are superfluous as we are in
> *UB-land* if the requirements above are not met.
> 
> See `[unique.ptr.single.ctor]p9-12` for more information.
> 

So if I understand correctly, the correct noexcept expression would be something along the line of

noexcept( std::is_reference<T>::value == false || std::is_lvalue_reference<decltype(Releaser)>::value == true )

And probably a static_assert should be added to warrant that in case of a non-ref type, copy/move ctors is non-throwing?

> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel

Thanks,

-- 
  Hugo Beauzée-Luyssen
  hugo at beauzee.fr