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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:541-542
   auto __destruct_first = __first2;
-  try {
-#endif
+  auto __guard =
+      std::__make_exception_guard(_AllocatorDestroyRangeReverse<_Alloc, _Iter2>(__alloc, __destruct_first, __first2));
   while (__first1 != __last1) {
----------------
philnik wrote:
> Mordante wrote:
> > This really makes it a lot easier to review and maintain.
> This still doesn't work, since it uses CTAD.
Shouldn't this work since we have `_LIBCPP_CTAD_SUPPORTED_FOR_TYPE`?


================
Comment at: libcxx/include/__utility/exception_guard.h:78
         __completed_ = true;
     }
 
----------------
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.



================
Comment at: libcxx/test/libcxx/utilities/exception_guard.pass.cpp:24
             auto rollback = [&] { rolled_back = true; };
-            std::__transaction<decltype(rollback)> t(rollback);
+            std::__exception_guard<decltype(rollback)> t(rollback);
         }
----------------
Might want to rename `t` to `g` in the tests here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133661