[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/include/__utility/transaction.h:55
+    _LIBCPP_HIDE_FROM_ABI
+    _LIBCPP_CONSTEXPR explicit __transaction(_Rollback __rollback)
+        : __rollback_(_VSTD::move(__rollback))
----------------
Mordante wrote:
> Quuxplusone wrote:
> > Mordante wrote:
> > > I see a lot of different `constexpr` macros in this class
> > > `_LIBCPP_CONSTEXPR`, `_LIBCPP_CONSTEXPR_AFTER_CXX11` and `_LIBCPP_CONSTEXPR_AFTER_CXX17`. Looking at this I expect the class not be usable as constexpr before C++20, due to the `constexpr` macro for the destructor.
> > This is presumably due to a style guideline that I made up ;) a while ago: "use the most aggressive constexpr macro possible for each place in internal code," so that if we ever drop support for C++03 (or Clang gains support for `constexpr` as an extension in C++03 mode), we can replace the absolute maximum number of uses of `_LIBCPP_CONSTEXPR` in one fell swoop. I.e. I think this is fine.
> > 
> > But, given the intended usage of this class... do we need it to be constexpr at all? Certainly not for `call_once` or `uninitialized_copy_n`. @ldionne, could you add some sample usages (in places like <algorithm> and the various containers, maybe) that show why constexprness is a good idea? Or if no great examples are conspicuous, I'd recommend removing the constexpr keywords entirely for now.
> I wasn't aware of this new style and haven't noticed it before during reviews. To me it feels odd especially when the destructor is the "limiting factor". To be honest I personally don't like this style.
> 
> That being said, if this is the style we've used before, then I withdraw my earlier comment about mixing the macros.
The rule makes a lot //more// sense (IMO) when it's talking about some random free function, like e.g. `__sift_up` or `__sort3` (constexpr-after-11, even though their only callers are constexpr-after-17). I'll gladly agree that a class with a bunch of member functions at different levels is a pretty pathological case for this rule (although I'll be at-best-ambivalent as to whether that means we should abandon the rule even in this pathological case).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115730