[libcxx-commits] [PATCH] D81954: Remove the try/catch codepath if `swap` is `noexcept`.

[Michael Schellenberger Costa via Phabricator via libcxx-commits]

miscco added inline comments.


================
Comment at: libcxx/include/variant:1046
   inline _LIBCPP_INLINE_VISIBILITY
   void __swap(__impl& __that)  {
     if (this->valueless_by_exception() && __that.valueless_by_exception()) {
----------------
zoecarver wrote:
> ldionne wrote:
> > mpark wrote:
> > > mpark wrote:
> > > > mclow.lists wrote:
> > > > > ldionne wrote:
> > > > > > zoecarver wrote:
> > > > > > > I think it would be good to also mark this `noexcept(__all<(is_nothrow_move_constructible_v<_Types> && is_nothrow_swappable_v<_Types>)...>::value)`.
> > > > > > Yeah, I was going to suggest the same thing. If it makes sense. If not, why?
> > > > > You pretty much have to do that, otherwise you get the try/catch/terminate block in the caller
> > > > I would actually prefer to not mark this `noexcept`, because trying to propagate `noexcept` down doesn't really scale.
> > > > As in, if we mark this with the `noexcept` condition, we'd also, with the same principle, mark `__generic_construct`,
> > > > `__visit_alt_at`, and essentially propagate it down to all of the helper functions like `__assign`, etc.
> > > Hm, I don't think this is true based on https://gcc.godbolt.org/z/68_ZWp
> > I'm satisfied with this argument.
> Given the Godbolt link, I'm OK not marking this `noexcept`. But, I would still rather we mark it `noexcept` and, for what it's worth, I think we should probably also mark all those functions `noexcept`. IMO it's better to mark as many functions as possible with `noexcept` as long as the standard allows it (and that seems to be what we do in most places). For example, if we use `__swap` in another function/member internally and that function isn't marked `noexcept`, we have suddenly added a lot of bloat for _no reason at all_.
While I would also prefere to put `noexcept` on every little helper it feels out of scope for this bug fix. That is more of a larger audit of the whole thing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81954