[libcxx-commits] [PATCH] D133661: [libc++] Improve binary size when using __transaction

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__utility/exception_guard.h:78
         __completed_ = true;
     }
 
----------------
ldionne wrote:
> philnik wrote:
> > Mordante wrote:
> > > When this class is intended to be an exception guard it doesn't need this function nor the `__completed_` member. Instead we can do something along the lines of:
> > > ```
> > > #ifndef _LIBCPP_NO_EXCEPTIONS
> > > #if _LIBCPP_STD_VER > 14
> > >   _LIBCPP_HIDE_FROM_ABI bool __need_rollback() { return uncaught_exceptions(); } 
> > > #else
> > >   _LIBCPP_HIDE_FROM_ABI bool __need_rollback() { return uncaught_exception(); } 
> > > #endif
> > > 
> > > 
> > >  _LIBCPP_CONSTEXPR_SINCE_CXX20 ~__exception_guard() {
> > >     if(__need_rollback())
> > >       __rollback_();
> > >   }
> > > #endif 
> > > ```
> > > With some extra boilerplate to add `_LIBCPP_CONSTEXPR_SINCE_CXX20`.
> > > 
> > > Then we can even use most of this class when `_LIBCPP_NO_EXCEPTIONS` is defined. Then it just does an extra store of the never used `__rollback_` member.
> > This would break when running in a destructor though, right? Then `uncaught_exception()` might return true even if we don't actually want to roll back a transaction. That's the whole reason `uncaught_exceptions()` was added.
> IMO we should strive to keep the scope that the `__exception_guard` is guarding explicit. If you use `uncaught_exceptions` (or other) to detect whether an exception was thrown, then you'd have to use the class as
> 
> ```
> { // begin new scope
>   __exception_guard guard = ...;
>   // actions...
> } // end scope explicitly here
> ```
> 
> This may not always be convenient. Instead, being able to explicitly say "okay, here the thing I was guarding for exceptions is done" makes the code clearer.
> 
I just had a discussion with Mark where he explained why he wasn't a big fan of the current behavior, where `exception_guard` will behave differently whether exceptions are enabled or not. I think we should change the no-exceptions version to

```
template <class _Rollback>
struct __exception_guard {
  __exception_guard() = delete;
  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG __exception_guard(_Rollback) {}
  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG __exception_guard(__exception_guard&&)
      _NOEXCEPT_(is_nothrow_move_constructible<_Rollback>::value) {}
  __exception_guard(__exception_guard const&) = delete;
  __exception_guard& operator=(__exception_guard const&) = delete;
  __exception_guard& operator=(__exception_guard&&) = delete;

  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG void __complete() _NOEXCEPT { __completed_ = true; }
  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG ~__exception_guard() { _LIBCPP_ASSERT(__completed_); }

private:
  bool __completed_;
};
```

This way, we enforce the intended usage of the class -- when exceptions are disabled, `__complete()` should always get called. And concretely this will get optimized away.

The reason I'm not a fan of depending on `uncaught_exceptions()` is that the function is defined in `libc++abi.dylib`. I am concerned about adding a call to a function hidden behind an ABI boundary that the optimizer doesn't know anything about, when instead we could just be setting and checking a bool visible to the optimizer in this very scope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133661