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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked 5 inline comments as done.
philnik added inline comments.


================
Comment at: libcxx/include/__utility/transaction.h:90-104
+#else
+template <class _Rollback>
+struct __transaction {
+  __transaction() = delete;
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG __transaction(_Rollback) {}
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG __transaction(__transaction&&)
+      _NOEXCEPT_(is_nothrow_move_constructible<_Rollback>::value) {}
----------------
ldionne wrote:
> Mordante wrote:
> > huixie90 wrote:
> > > having this `__transaction` class as a noop class in `_LIBCPP_NO_EXCEPTIONS` mode might cause confusions, because the class name does not indicate anything to do with exceptions (and could be misused as a general purpose `scope_guard`).
> > > 
> > > what about having a function `__make_exception_guard`, which returns the `__transaction` class in the exception mode, and returns a `__noop_transaction` class in the noexception mode?
> > +1 for the confusion. What makes it more confusing is the usage of `_NOEXCEPT` in this class. Isn't that useless when exceptions are disabled? Please also some comment why this class it to be used as a no-op.
> > 
> > I'm not even sure whether we really want this class. I expect a transaction that is not committed when it goes out of scope to do a rollback.
> > ```
> > void __foo() {
> >   __transaction __t;
> >   ... // do stuff
> >   if(!__happy())
> >     return;
> >   __t.__complete();
> > }
> > 
> > ```
> > In that way this class behaves different depending on whether or not exceptions are enabled.
> > 
> > Based on the failure in the 'No exception' CI the behaviour above was intended to be working.
> I fully agree with the other reviewers, I think it would be wrong to make `__transaction` a no-op based on whether exceptions are enabled. However, I think it's only an issue because of the semantics associated to the name `__transaction`, which does not imply something related to exceptions.
> 
> I'd suggest:
> 
> ```
> template <class _Rollback>
> struct __noop_transaction {
>   __noop_transaction() = delete;
>   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG __noop_transaction(_Rollback) {}
>   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG __noop_transaction(__transaction&&)
>       _NOEXCEPT_(is_nothrow_move_constructible<_Rollback>::value) {}
>   __noop_transaction(const __transaction&) = delete;
>   __noop_transaction& operator=(const __transaction&) = delete;
>   __noop_transaction& operator=(__transaction&&) = delete;
> 
>   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG void __complete() _NOEXCEPT {}
>   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG ~ __noop_transaction() {}
> };
> 
> 
> #ifndef _LIBCPP_NO_EXCEPTIONS
> template <class _Rollback>
> using __exception_guard = __transaction<_Rollback>;
> #else
> template <class _Rollback>
> using __exception_guard = __noop_transaction<_Rollback>;
> #endif
> 
> template <class _Rollback>
> _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR __exception_guard<_Rollback> __make_exception_guard(_Rollback __rollback) {
>   return __exception_guard <_Rollback>(std::move(__rollback));
> }
> ```
> 
> Also, you should use `__exception_guard` instead of `__transaction` in most existing places where `__transaction` is used right now.
`_NOEXCEPT` makes a difference, even when exceptions are disabled. `noexcept(expr)` still isn't always true.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133661