This is an archive of the discontinued LLVM Phabricator instance.

Remove the try/catch codepath if `swap` is `noexcept`.
ClosedPublic

Authored by mpark on Jun 16 2020, 10:58 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGada2a8ea4a9c: Remove the try/catch codepath if `swap` is `noexcept`.
Summary

In the case where swap is noexcept, we should avoid the extension to provide strong-exception guarantee.

Fixes https://bugs.llvm.org/show_bug.cgi?id=46342

Diff Detail

Event Timeline

mpark created this revision.Jun 16 2020, 10:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2020, 10:58 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
mpark retitled this revision from Remove the try/catch codepath if swap is noexcept. to Remove the try/catch codepath if `swap` is `noexcept`..Jun 16 2020, 11:00 AM
mpark edited the summary of this revision. (Show Details)

Thanks for making this patch. Other than the noexcept issue, this LGTM.

libcxx/include/variant
1046

I think it would be good to also mark this noexcept(__all<(is_nothrow_move_constructible_v<_Types> && is_nothrow_swappable_v<_Types>)...>::value).

1072

I think this is OK but, are there any compilers that we support that don't support if constexpr in C++17?

This LGTM. It would be great to add a test though. Would it make sense to have a test under libcxx/test/libcxx that checks the codegen like we do in several places in Clang?

libcxx/include/variant
1046

Yeah, I was going to suggest the same thing. If it makes sense. If not, why?

miscco added a subscriber: miscco.Jun 16 2020, 11:17 AM

I would second @ldionne that we should add some static_assert(noexcept(...) tests

libcxx/include/variant
1065

Should that be __ugly?

mclow.lists added inline comments.
libcxx/include/variant
1046

You pretty much have to do that, otherwise you get the try/catch/terminate block in the caller

mpark marked 2 inline comments as done.Jun 16 2020, 11:38 AM
mpark added inline comments.
libcxx/include/variant
1046

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.

1072

Not that I know of...

mpark updated this revision to Diff 271153.Jun 16 2020, 11:38 AM

Uglify is_noexcept variable.

mpark marked an inline comment as done.Jun 16 2020, 11:39 AM

@ldionne I didn't know we had a mechanism to check codegen. Could you point me to an example?

mpark marked an inline comment as done.Jun 16 2020, 11:45 AM
mpark added inline comments.
libcxx/include/variant
1046

Hm, I don't think this is true based on https://gcc.godbolt.org/z/68_ZWp

@ldionne I didn't know we had a mechanism to check codegen. Could you point me to an example?

For example clang/test/CodeGenCXX/blocks-cxx11.cpp (code). This would mean adding a dependency on FileCheck, but that might be OK anyway. WDYT?

libcxx/include/variant
1046

I'm satisfied with this argument.

zoecarver added inline comments.Jun 16 2020, 12:19 PM
libcxx/include/variant
1046

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_.

@ldionne I didn't know we had a mechanism to check codegen. Could you point me to an example?

For example clang/test/CodeGenCXX/blocks-cxx11.cpp (code). This would mean adding a dependency on FileCheck, but that might be OK anyway. WDYT?

Geez, I'm trying to add the ability to run FileCheck inside libc++ tests and it's a pain. Let's not block this fix on that.

ldionne accepted this revision.Jun 16 2020, 12:25 PM
This revision is now accepted and ready to land.Jun 16 2020, 12:25 PM
miscco added inline comments.Jun 16 2020, 12:50 PM
libcxx/include/variant
1046

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.

This revision was automatically updated to reflect the committed changes.
lewissbaker added inline comments.
libcxx/include/variant
1070

Why is the is_nothrow_swappable_v requirement checked here?
The code below is just calling generic_construct and move_nothrow, neither of which seem to call swap().

mpark marked 2 inline comments as done.Jul 6 2020, 7:12 PM
mpark added inline comments.
libcxx/include/variant
1070

Thanks! I've followed up with https://reviews.llvm.org/D83274