Re: [scope] Scope review starts on November 26th

From: Alexander Grund (alexander.grund_at_[hidden])
Date: 2023-12-01 08:32:07

Am 30.11.23 um 18:49 schrieb Andrey Semashev via Boost:
> 3. #ifdef BOOST_HAS_PRAGMA_ONCE : why bother?
> Why not?
It is extra code without any benefit, isn't it?
>> 6. include/boost/scope/detail/is_not_like.hpp: no human readable
>> explanation of what this type-trait is supposed to do.
> It tests whether the left-hand type is not an instantiation of the
> right-hand template.
I think the point was that this explanation should be in the file.
> I don't want to include <functional> just for std::reference_wrapper.
> I don't want to include <algorithm> just for std::swap.
> I don't want to include <utility> just for std::move.
> I don't want to include <functional> just for that trivial function
> object wrapper.
I collected all such statements from your replies and would like to 
object: Reimplementing std-library components to avoid an include looks 
very wrong to me.

Not only is it likely premature optimization: User code including your 
headers likely included those files already making the include "free". 
Especially as many std-headers transitively include <utility> or even 
<algorithm>.
And 2nd: With the collection above there are now 2 points obvious that 
<functional> is not included "just for x" and then "just for y". So now 
you have 2 reasons already to include it.
And finally this is about readability: Everyone knows what 
`std::move(that)` does, but it is not that obvious for `static_cast< 
unique_resource_data&& >(that)` Similar for swapping "m_allocated" being 
expanded to 3 lines instead of `std::swap` obfuscating what it does. And 
ref_wrapper begs the question if and how it is different to 
std::reference_wrapper

TLDR: I'd strongly suggest to use the std-library where possible to a) 
not reinvent the wheel and b) improve readability by using existing 
idioms/names.

And a possible bug I've just seen: template< typename R, typename D,...> 
explicit unique_resource_data(R&& res, D&& del)... : 
unique_resource_data(static_cast< R&& >(res), static_cast< D&& >(del), 
traits_type::is_allocated(static_cast< R&& >(res))) { }

The `static_cast<R&&>` look wrong. Should that be a std::move or 
std::forward? It is std::forward, but I had to check to be sure. (see 
above where a similar cast was a std::move) To me it looks like 
it`is_allocated` should not have a cast, should it? What if it is 
implemented to accept an instance, you castit to an rvalue, i.e. move it 
into that call and then use the moved-from value to construct 
`unique_resource_data` Similar for the deleter where using std::forward 
makes the intent clear.

So please use the existing vocabulary types/functions/idioms.

Alex