Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid new warning C5267 for deprecated implicit copy ctor/assign #3497

Merged
merged 2 commits into from
Feb 26, 2023

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Feb 23, 2023

@JonCavesMSFT just implemented a new off-by-default warning for VS 2022 17.7 Preview 1 (internal MSVC-PR-452406) that some internal teams are very interested in using. This warns about the deprecated case in WG21-N4928 [class.copy.ctor]/6 (emphasis mine):

If the class definition does not explicitly declare a copy constructor, a non-explicit one is declared implicitly. If the class definition declares a move constructor or move assignment operator, the implicitly declared copy constructor is defined as deleted; otherwise, it is defaulted (9.5). The latter case is deprecated if the class has a user-declared copy assignment operator or a user-declared destructor (D.8).

and [class.copy.assign]/2 (emphasis mine):

If the class definition does not explicitly declare a copy assignment operator, one is declared implicitly. If the class definition declares a move constructor or move assignment operator, the implicitly declared copy assignment operator is defined as deleted; otherwise, it is defaulted (9.5). The latter case is deprecated if the class has a user-declared copy constructor or a user-declared destructor (D.8).

The warning (which uses the same number for copy ctors and copy assigns) looks like:

warning C5267: definition of implicit copy constructor for 'X' is deprecated because it has a user-provided destructor
warning C5267: definition of implicit assignment operator for 'X' is deprecated because it has a user-provided copy constructor

We can't enable the warning until 17.7 Preview 1 ships, but we can clean up the product and test code now. (When we enable the warning, the proper places to do so will likely be tests/std/tests/prefix.lst and tests/tr1/prefix.lst, unless libcxx receives numerous cleanups, in which case tests/universal_prefix.lst will be the place.)

I have filed VSO-1753270 "/clr C++20 alignas with defaulted SMFs emits fatal error C1193: an error expected in ...\yyaction.cpp(2976) not reached", which prevents one type from being cleaned up here (aligned_type in P0220R1_any).

Notes

  • bitset
    • This is the only affected product code that I found.
    • Copy ctors are ABI-relevant (did you ever hear the tragedy of Darth tuple<> The Wise?). However, this should be ABI-safe because the copy ctor is being defaulted and therefore remains trivial.
    • I marked this as _CONSTEXPR23 to match the WP. (It is implicitly noexcept.)
    • One unrelated cleanup affecting nearby but different code: move reference::flip to match the WP's order.
  • P0220R1_any
    • I've chosen to user-provide copy ctors that increment count, to keep it balanced with the destructor. They must be marked noexcept so that these types remain eligible for any's Small Object Optimization:

      STL/stl/inc/any

      Lines 51 to 53 in 86acfb1

      template <class _Ty>
      inline constexpr bool _Any_is_small = alignof(_Ty) <= alignof(max_align_t)
      && is_nothrow_move_constructible_v<_Ty> && sizeof(_Ty) <= _Any_small_space_size;
  • P0220R1_optional
    • The last X also needs a defaulted default ctor (otherwise the defaulted copy ctor will suppress it).
  • P0608R3_improved_variant_converting_constructor
    • convertible_bool didn't need a defaulted destructor at all, so I have simply dropped it.
  • P0674R1_make_shared_for_arrays
    • Similarly, WeirdDeleter needs a defaulted default ctor.
  • P0784R7_library_support_for_more_constexpr_containers
    • I've chosen to mark these defaulted functions as constexpr and noexcept for local consistency, although I could be persuaded otherwise.
  • P0896R4_views_join
    • I noticed that non_trivially_destructible_input_iterator didn't have any way to construct a fresh object (it intentionally deletes its default ctor below). I've seen compilers warn about that, so it seems prudent to introduce a hypothetical ctor, which I've commented.

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Feb 23, 2023
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner February 23, 2023 23:54
@github-actions github-actions bot added this to Initial Review in Code Reviews Feb 23, 2023
@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in Code Reviews Feb 23, 2023
Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I've submitted https://reviews.llvm.org/D144694 to mirror changes to portions of the any and optional tests that come from libc++. (Including my suggestions.)

stl/inc/bitset Outdated Show resolved Hide resolved
tests/std/tests/P0220R1_any/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0220R1_any/test.cpp Outdated Show resolved Hide resolved
Code Reviews automation moved this from Final Review to Work In Progress Feb 24, 2023
@CaseyCarter CaseyCarter moved this from Final Review to Ready To Merge in Code Reviews Feb 24, 2023
@StephanTLavavej StephanTLavavej self-assigned this Feb 24, 2023
@StephanTLavavej
Copy link
Member Author

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit c90b84a into microsoft:main Feb 26, 2023
Code Reviews automation moved this from Ready To Merge to Done Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants