[libcxx-commits] [PATCH] D115730: [libc++] Add a helper class to write code with the strong exception guarantee

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__utility/transaction.h:52
+struct __transaction {
+    __transaction() = delete;
+
----------------
Mordante wrote:
> Why not omit this?
I like calling out explicitly the properties of types instead of relying too much on the implicit rules, because these rules are pretty complicated. For example, see how I explicitly delete the copy constructor and the assignment operators below -- I could have relied on any number of implicit rules (e.g. the fact that we define a destructor explicitly) to get the same effect, but IMO it forces the reader to be more cleaver than I want.

LMK if you are not convinced and I'll remove it.


================
Comment at: libcxx/include/__utility/transaction.h:55
+    _LIBCPP_HIDE_FROM_ABI
+    _LIBCPP_CONSTEXPR explicit __transaction(_Rollback __rollback)
+        : __rollback_(_VSTD::move(__rollback))
----------------
Quuxplusone wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > Mordante wrote:
> > > > I see a lot of different `constexpr` macros in this class
> > > > `_LIBCPP_CONSTEXPR`, `_LIBCPP_CONSTEXPR_AFTER_CXX11` and `_LIBCPP_CONSTEXPR_AFTER_CXX17`. Looking at this I expect the class not be usable as constexpr before C++20, due to the `constexpr` macro for the destructor.
> > > This is presumably due to a style guideline that I made up ;) a while ago: "use the most aggressive constexpr macro possible for each place in internal code," so that if we ever drop support for C++03 (or Clang gains support for `constexpr` as an extension in C++03 mode), we can replace the absolute maximum number of uses of `_LIBCPP_CONSTEXPR` in one fell swoop. I.e. I think this is fine.
> > > 
> > > But, given the intended usage of this class... do we need it to be constexpr at all? Certainly not for `call_once` or `uninitialized_copy_n`. @ldionne, could you add some sample usages (in places like <algorithm> and the various containers, maybe) that show why constexprness is a good idea? Or if no great examples are conspicuous, I'd recommend removing the constexpr keywords entirely for now.
> > I wasn't aware of this new style and haven't noticed it before during reviews. To me it feels odd especially when the destructor is the "limiting factor". To be honest I personally don't like this style.
> > 
> > That being said, if this is the style we've used before, then I withdraw my earlier comment about mixing the macros.
> The rule makes a lot //more// sense (IMO) when it's talking about some random free function, like e.g. `__sift_up` or `__sort3` (constexpr-after-11, even though their only callers are constexpr-after-17). I'll gladly agree that a class with a bunch of member functions at different levels is a pretty pathological case for this rule (although I'll be at-best-ambivalent as to whether that means we should abandon the rule even in this pathological case).
My original intent was to make things constexpr as early as possible, but I agree this ends up being somewhat messy. Instead, I've made everything be constexpr in C++20 and above only.

The benefit of making this class constexpr is that it can be used inside functions that are themselves constexpr (there's more than just the `uninitialized_foo` family that will benefit from this). If we want to use it in a C++11/C++14/C++17 constexpr function, we can come back and try to make it work.

@Quuxplusone An example would be `std::vector::insert`. Like I said, we can come back later and make it constexpr friendly in older standards if we have a use for it, but I wouldn't de-constexprify this upfront.


================
Comment at: libcxx/include/__utility/transaction.h:62
+    _LIBCPP_CONSTEXPR_AFTER_CXX11 __transaction(__transaction&& __other)
+        _NOEXCEPT_(is_nothrow_move_constructible<_Rollback>::value)
+        : __rollback_(_VSTD::move(__other.__rollback_))
----------------
Mordante wrote:
> The tests require C++11 or later.
The tests do, however we're still going to use the class in places that are supported in C++03, like some algorithms, so I can't use raw `noexcept`.


================
Comment at: libcxx/include/__utility/transaction.h:80
+    _LIBCPP_CONSTEXPR_AFTER_CXX17 ~__transaction() {
+        if (!__completed_)
+            __rollback_();
----------------
DanielMcIntosh-IBM wrote:
> Would it be possible to get rid of `__complete()` by replacing this with something like `if (std::uncaught_exceptions() > initial_exception_count)` and initializing `initial_exception_count = std::uncaught_exceptions()` in the constructor?
Yes, however as Arthur explained in another comment (https://reviews.llvm.org/D115730?id=394248#inline-1106466), we don't want to add a call to `uncaught_exceptions()` on the success path. The benefit of the current approach is that the compiler will be able to see that we call `__complete()` on all happy paths, and as a result it should be able to do optimal codegen.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115730/new/

https://reviews.llvm.org/D115730