In the case where swap is noexcept, we should avoid the extension to provide strong-exception guarantee.
Details
- Reviewers
ldionne - Group Reviewers
Restricted Project - Commits
- rGada2a8ea4a9c: Remove the try/catch codepath if `swap` is `noexcept`.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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? |
libcxx/include/variant | ||
---|---|---|
1046 | You pretty much have to do that, otherwise you get the try/catch/terminate block in the caller |
libcxx/include/variant | ||
---|---|---|
1046 | I would actually prefer to not mark this noexcept, because trying to propagate noexcept down doesn't really scale. | |
1072 | Not that I know of... |
@ldionne I didn't know we had a mechanism to check codegen. Could you point me to an example?
libcxx/include/variant | ||
---|---|---|
1046 | Hm, I don't think this is true based on https://gcc.godbolt.org/z/68_ZWp |
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_. |
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.
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. |
libcxx/include/variant | ||
---|---|---|
1070 | Why is the is_nothrow_swappable_v requirement checked here? |
libcxx/include/variant | ||
---|---|---|
1070 | Thanks! I've followed up with https://reviews.llvm.org/D83274 |
I think it would be good to also mark this noexcept(__all<(is_nothrow_move_constructible_v<_Types> && is_nothrow_swappable_v<_Types>)...>::value).