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

From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2023-12-01 12:26:18

On 12/1/23 11:32, Alexander Grund via Boost wrote:
> 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?

Whether there is any benefit depends on the compiler implementation. It
doesn't cost me much to add three lines per header, if it may improve
compile speeds in some cases.

I don't feel strongly enough about this, so if there are objections from
the users or reviewers, I can remove them. I just don't see the reason to.

>>> 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 can add a comment, but I reiterate that this is an implementation detail.

>> 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".

This depends on user's code. I have use cases where scope guards are
used in very lightweight public headers (where functions are inline for
performance reasons), so every included header matters.

To put it another way, Boost.Scope header includes set the lowest bar of
dependencies that the user's code can have. I see value in setting that
bar as low as possible, without having to add unreasonable amount of
code duplication. That's why I'm including <type_traits>, for example -
because I use many components from it, and in many places.

> Especially as many std-headers transitively include <utility> or even
> <algorithm>.

That would be a QoI matter. Most standard libraries include more
fine-grained private headers internally to only pull the components they
need for the purpose of the public headers.

> 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.

Those two instances are in different and unrelated headers.
Specifically, in scope_success.hpp I need a negation function object,
and in unique_resource.hpp I need a lightweight reference wrapper. In
the latter case, not even full reference_wrapper, but something that
will just invoke operator() on a pointer.

Frankly, when I wrote that code I was considering using
boost::reference_wrapper instead. But unfortunately it doesn't support
operator(), and also includes extra bells and whistles that I don't
need, so I just decided to write a simple wrapper myself. If it had
operator(), I would probably use boost::reference_wrapper.

> 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

I'm willing to exchange some amount of readability for potentially
improving compile times. As the maintainer, I think, I'm allowed to make
that tradeoff.

> 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.

An a general comment regarding the use (or not use) of the standard
library. I do not consider the standard library a holy cow of some sort,
and I will not use its components just because it's standard library. I
have said it before, and I'll reiterate that I will consider my options
before using another library, including the standard library. One of
these options is implementing the component myself, and sometimes, this
option is the most appealing. I'm not going to disregard that option
"just because".

> 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.

Thanks, I will take a look at that code. Quite possibly you're right,
and I should avoid forwarding to is_allocated.