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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante requested changes to this revision.
Mordante added a comment.
This revision now requires changes to proceed.

In general quite happy with the current form! But I think I spotted a bug, and found some minor nits.

Can you also update the title of the patch.



================
Comment at: libcxx/include/__utility/exception_guard.h:29
+// transaction, it will be rolled back automatically. When the transaction is done, one
+// must mark it as being complete so it isn't rolled back when the transaction is destroyed.
+//
----------------
replace transaction with exception guard/__exception_guard in this paragraph.


================
Comment at: libcxx/include/__utility/exception_guard.h:40
+// layer with exceptions enabled seems a lot less common, especially one that tries to catch an exception through
+// -fno-exceptions code.
+//
----------------
Nit: I prefer comments to stay in the 80 column limit. These long lines are less easy to read. For a single line it's not too bad, but this is quite a bit of text.


================
Comment at: libcxx/include/__utility/exception_guard.h:40
+// layer with exceptions enabled seems a lot less common, especially one that tries to catch an exception through
+// -fno-exceptions code.
+//
----------------
Mordante wrote:
> Nit: I prefer comments to stay in the 80 column limit. These long lines are less easy to read. For a single line it's not too bad, but this is quite a bit of text.
Maybe copy this paragraph in the commit message.


================
Comment at: libcxx/include/__utility/exception_guard.h:58
+//    }
+//
+
----------------
Nice comment!


================
Comment at: libcxx/include/__utility/exception_guard.h:67
+    _LIBCPP_CONSTEXPR_SINCE_CXX20 explicit __exception_guard(_Rollback __rollback)
+        : __rollback_(_VSTD::move(__rollback))
+        , __completed_(false)
----------------
Since this is a new file can you s/_VSTD/std/ in this file?


================
Comment at: libcxx/include/__utility/exception_guard.h:105
+  _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;
----------------
I think there is a bug here. The normal move sets `__other.__completed_` this one doesn't.
I think there should be a test for this case too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133661