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

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