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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/libcxx/detail/transaction.pass.cpp:125
+    return 0;
+}
----------------
ldionne wrote:
> Quuxplusone wrote:
> > I'd also like to see that it works properly when called from a destructor. (This is more obviously a big deal for my alternative `__on_exception_rollback` design, but might as well check it no matter which design we pick.)
> > ```
> > bool rolled_back = false;
> > struct S {
> >     ~S() {
> >         std::__transaction t([&]{ rolled_back = true; });
> >         t.__set_completed();
> >     }
> > };
> > try {
> >     S s;
> >     throw 0;
> > } catch (...) {
> >     assert(!rolled_back);
> > }
> > ```
> > (I'd say if `S` has a //data member// of type `__transaction`, that produces weird behavior but also violates the intended usage of the type, and so we shouldn't worry about //that// case.)
> Adding the test.
> 
> Sorry, what does `__on_exception_rollback` do? Does it check whether the destructor is being called during stack unwinding?
Yeah, exactly what @DanielMcIntosh-IBM just said elsewhere in this review. :)
https://godbolt.org/z/78vrfj8e7
The bad thing about `uncaught_exceptions` is that compilers don't understand that its value never changes by ordinary means, so when the `__on_exception_rollback` object is destroyed in the ordinary way at the end of the scope, it causes codegen. Actually, that might make `__on_exception_rollback` a complete non-starter, unfortunately. We can't have something in a random library scope that unconditionally adds //two// calls to `std::uncaught_exceptions` on the success path. :(

However, taking another page from the `Auto.h` playbook, I think you should consider eliminating the `completed_` member altogether and just making the lambda always run on destruction. If the programmer wants the lambda's behavior to be conditional, they can just... put an `if` in it.
```
    bool completed = false;
    __on_scope_exit guard([&]() {
        if (!completed) puts("Rolling back!");
    });
    puts("Starting transaction");
    f();
    completed = true;
    // guard runs here
```
IMO, this is a more readable API than having a magic `.set_completed()` method on the guard object //itself//.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115730