Dear all, a user noticed that some classes in Boost.Histogram do not provide a strong exception guarantee, although with a few changes they could. I never promised that guarantee for those classes, but I am inclined to support it. To that end, I am considering use a type called delayed_forward to the public interface of that class. I would like to get a mini-review on the concept. The change is backward-compatible and it offers some advantages, so it seems like a good solution to me, but maybe I am missing some caveats. The constructor of my class, let’s call it Foo, accepts some potentially non-trivial parameters by value, because I didn’t want to write an interface with a templated forward reference. Passing non-trivial parameters by value is cheap if the compiler can do RVO or if the user calls the move constructor via std::move. Not using a forward reference saves a few template instantiations and thus reduces compile time. I think I got that from Herb Sutter in a GotW entry, but I cannot find it right now. With delayed_forward one can efficiently pass values to Foo with a single ctor. The trade-off is a tiny runtime cost, but that’s ok if we know that Foos are not created en mass in a hot loop. That already guided my choice of passing by value. A minimal demo is here: https://godbolt.org/z/z71E1jno1 The design of delayed_forward is so simple that it probably already exists somewhere, likely under another name. Maybe even in Boost. What do you think? Hans
Hans Dembinski wrote: ...
With delayed_forward one can efficiently pass values to Foo with a single ctor. The trade-off is a tiny runtime cost, but that’s ok if we know that Foos are not created en mass in a hot loop. That already guided my choice of passing by value.
A minimal demo is here: https://godbolt.org/z/z71E1jno1
It will be better if you show how the existing code (before the delayed_forward transformation) looks like. You could also point to the actual code in Histogram for those who would like even more context. If I understand correctly, the lack of strong guarantee is caused by the by-value constructor moving from the argument even when construction fails? E.g. Foo foo( true, std::move( my_t ) ); // leaves my_t moved-from on exception
On 10/01/2023 07:15, Peter Dimov wrote:
If I understand correctly, the lack of strong guarantee is caused by the by-value constructor moving from the argument even when construction fails? E.g.
Foo foo( true, std::move( my_t ) ); // leaves my_t moved-from on exception
If so, that doesn't seem like an unusual feature -- https://clang.llvm.org/extra/clang-tidy/checks/modernize/pass-by-value.html strongly encourages that constructors accept movable values by value, which will always move-from them even if the constructor later throws. (If the parameter type's move/copy constructor throws then it gets more complicated, though move constructors at least are typically expected to not throw.)
On 9. Jan 2023, at 23:13, Gavin Lambert via Boost <boost@lists.boost.org> wrote:
If so, that doesn't seem like an unusual feature -- https://clang.llvm.org/extra/clang-tidy/checks/modernize/pass-by-value.html strongly encourages that constructors accept movable values by value, which will always move-from them even if the constructor later throws. (If the parameter type's move/copy constructor throws then it gets more complicated, though move constructors at least are typically expected to not throw.)
Thanks for that pointer. Yes, this was the advice I meant.
If so, that doesn't seem like an unusual feature -- https://clang.llvm.org/extra/clang-tidy/checks/modernize/pass-by-value.html strongly encourages that constructors accept movable values by value, which will always move-from them even if the constructor later throws. (If the parameter type's move/copy constructor throws then it gets more complicated, though move constructors at least are typically expected to not throw.) I'd also agree with that as I favor consistency. So after something `foo(std::move(bar))` I'd always assume that `bar` can no longer be used, I mean that's the point, isn't it? The `delayed_forward`, especially as hidden from the call site, voids my assumption: How would I know when exactly I can still use bar? I'd need to consult the documentation and address potential version differences for something fundamental.
So consistency here means: The target of a move operation is always a (data) sink. Not a "maybe-sink". Alex
On 10. Jan 2023, at 12:16, Alexander Grund via Boost <boost@lists.boost.org> wrote:
If so, that doesn't seem like an unusual feature -- https://clang.llvm.org/extra/clang-tidy/checks/modernize/pass-by-value.html strongly encourages that constructors accept movable values by value, which will always move-from them even if the constructor later throws. (If the parameter type's move/copy constructor throws then it gets more complicated, though move constructors at least are typically expected to not throw.) I'd also agree with that as I favor consistency. So after something `foo(std::move(bar))` I'd always assume that `bar` can no longer be used, I mean that's the point, isn't it? The `delayed_forward`, especially as hidden from the call site, voids my assumption: How would I know when exactly I can still use bar? I'd need to consult the documentation and address potential version differences for something fundamental.
So consistency here means: The target of a move operation is always a (data) sink. Not a "maybe-sink”.
That’s a fair point, which I did not really address in Gavin’s comment. Is this the consensus here?
On Jan 10, 2023, at 3:16 AM, Alexander Grund via Boost <boost@lists.boost.org> wrote:
If so, that doesn't seem like an unusual feature -- https://clang.llvm.org/extra/clang-tidy/checks/modernize/pass-by-value.html strongly encourages that constructors accept movable values by value, which will always move-from them even if the constructor later throws. (If the parameter type's move/copy constructor throws then it gets more complicated, though move constructors at least are typically expected to not throw.) I'd also agree with that as I favor consistency. So after something `foo(std::move(bar))` I'd always assume that `bar` can no longer be used, I mean that's the point, isn't it? The `delayed_forward`, especially as hidden from the call site, voids my assumption: How would I know when exactly I can still use bar? I'd need to consult the documentation and address potential version differences for something fundamental.
So consistency here means: The target of a move operation is always a (data) sink. Not a "maybe-sink”.See std::
See `std::map::try_emplace(k, args…)`. https://en.cppreference.com/w/cpp/container/map/try_emplace Notes: Unlike insert or emplace, these functions do not move from rvalue arguments if the insertion does not happen. — Marshall
See `std::map::try_emplace(k, args…)`.
https://en.cppreference.com/w/cpp/container/map/try_emplace
Notes: Unlike insert or emplace, these functions do not move from rvalue arguments if the insertion does not happen.
Yes, however at the callsite you can clearly see the different semantics: "try_" That's much harder for a constructor. Although a well-named factory method might be a solution if this semantic is useful.
Alexander Grund wrote:
See `std::map::try_emplace(k, args…)`.
https://en.cppreference.com/w/cpp/container/map/try_emplace
Notes: Unlike insert or emplace, these functions do not move from rvalue arguments if the insertion does not happen.
Yes, however at the callsite you can clearly see the different semantics: "try_" That's much harder for a constructor. Although a well-named factory method might be a solution if this semantic is useful.
For a constructor example, see https://eel.is/c++draft/util.smartptr.shared.const#29.
On 9. Jan 2023, at 19:15, Peter Dimov via Boost <boost@lists.boost.org> wrote:
It will be better if you show how the existing code (before the delayed_forward transformation) looks like.
I added FooOld, which corresponds to the unsafe design I want to replace. https://godbolt.org/z/15q9EqnfP
You could also point to the actual code in Histogram for those who would like even more context.
Here is the related issue. https://github.com/boostorg/histogram/issues/379 The classes which we are discussing to improve are the axis classes. Several of these axis classes have throwing constructors. They all accept an optional Metadata in the ctor, which is user-defined and in general a non-trivial type. A comparably simple example is the integer axis. https://github.com/boostorg/histogram/blob/develop/include/boost/histogram/a...
If I understand correctly, the lack of strong guarantee is caused by the by-value constructor moving from the argument even when construction fails? E.g.
Foo foo( true, std::move( my_t ) ); // leaves my_t moved-from on exception
Yes. I added some code to main in godbolt to illustrate this.
Hans Dembinski wrote:
On 9. Jan 2023, at 19:15, Peter Dimov via Boost <boost@lists.boost.org> wrote:
It will be better if you show how the existing code (before the delayed_forward transformation) looks like.
I added FooOld, which corresponds to the unsafe design I want to replace. https://godbolt.org/z/15q9EqnfP
Thanks. I note that as written, FooOld actually has the same problem even if the argument is not taken by value. template <class T> struct FooOld2 { FooOld2(bool b, T const& targ) : t_(targ) { if (b) throw 1; } FooOld2(bool b, T&& targ = {}) : t_(std::move(targ)) { if (b) throw 1; } private: T t_; }; There might be a difference between passing by value or not, but for that, the exception would have to be thrown from a base or previous member initialization, and not the constructor body.
I added FooOld, which corresponds to the unsafe design I want to replace. https://godbolt.org/z/15q9EqnfP
Thanks. I note that as written, FooOld actually has the same problem even if the argument is not taken by value.
template <class T> struct FooOld2 { FooOld2(bool b, T const& targ) : t_(targ) { if (b) throw 1; } FooOld2(bool b, T&& targ = {}) : t_(std::move(targ)) { if (b) throw 1; } private: T t_; };
There might be a difference between passing by value or not, but for that, the exception would have to be thrown from a base or previous member initialization, and not the constructor body.
Yes, FooOld2 is also unsafe, but you can use a rollback guard to undo the move. FooOld3(bool b, T&& targ = {}) : t_(std::move(targ)) { rollback r(targ, t_); // moves t_ back to targ if not deactivated if (b) throw 1; r.deactivate(); } Or you can put all the throwing code into a lambda FooOld4(bool b, T&& targ = {}) : t_( [&](auto&& targ) { // to all the potentially throwing stuff here return targ; }(std::forward(targ)) ) { } The rollback version is also ok, but cannot be used when you pass by value. I don’t like the lambda version, it makes the code unreadable.
On 11/01/2023 22:35, Hans Dembinski wrote:
Yes, FooOld2 is also unsafe, but you can use a rollback guard to undo the move.
FooOld3(bool b, T&& targ = {}) : t_(std::move(targ)) { rollback r(targ, t_); // moves t_ back to targ if not deactivated if (b) throw 1; r.deactivate(); }
Note that a rollback implementation is still unsafe if the type in question has a noexcept(false) move constructor -- which, while unusual, is legal -- and might be more common than you think when it decays to use the copy constructor instead. (This can also be bad for performance, though only when throwing, so you likely don't care.) A common surprise is that copy-constructor-only types will still satisfy std::is_move_constructible_v, although typically not std::is_nothrow_move_constructible_v. (Which is one of the reasons why the "rule of five" exists now.) (You might ask why this is unsafe, since we only care about exceptions thrown in the constructor body -- if the initial parameter move/copy throws that's a different case, right? But consider that the initial parameter move/copy might succeed, the constructor body throws, and then the rollback move/copy also throws. This is especially likely if the constructor throw was a std::bad_alloc, but other cases are possible.) This isn't to say that the above is bad; but if you use it, don't forget to static_assert your assumptions (or provide alternative implementations).
Hans Dembinski wrote:
On 9. Jan 2023, at 19:15, Peter Dimov via Boost <boost@lists.boost.org> wrote:
It will be better if you show how the existing code (before the delayed_forward transformation) looks like.
I added FooOld, which corresponds to the unsafe design I want to replace. https://godbolt.org/z/15q9EqnfP
You could also point to the actual code in Histogram for those who would like even more context.
Here is the related issue. https://github.com/boostorg/histogram/issues/379
The classes which we are discussing to improve are the axis classes. Several of these axis classes have throwing constructors. They all accept an optional Metadata in the ctor, which is user-defined and in general a non-trivial type. A comparably simple example is the integer axis.
https://github.com/boostorg/histogram/blob/develop/include/boost/histogra m/axis/integer.hpp
If I understand correctly, the lack of strong guarantee is caused by the by-value constructor moving from the argument even when construction fails? E.g.
Foo foo( true, std::move( my_t ) ); // leaves my_t moved-from on exception
Yes. I added some code to main in godbolt to illustrate this.
I don't think I have any better ideas than what you did with delayed_forward. The irregularity of its operator(), where it sometimes assigns to the argument, and sometimes doesn't, isn't very elegant, but assigning a default-constructed value would be superfluous.
participants (5)
-
Alexander Grund -
Gavin Lambert -
Hans Dembinski -
Marshall Clow -
Peter Dimov