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

<xmemory>: remove deprecated std::allocator members in C++20 #1585

Merged
merged 3 commits into from
Feb 2, 2021
Merged

<xmemory>: remove deprecated std::allocator members in C++20 #1585

merged 3 commits into from
Feb 2, 2021

Conversation

MichaelRizkalla
Copy link
Contributor

This PR removes the deprecated std::allocator<void> and std::allocator's member functions and typedefs when compiling in C++20 mode.
It should close #1445

@MichaelRizkalla MichaelRizkalla requested a review from a team as a code owner January 23, 2021 16:27
@MichaelRizkalla
Copy link
Contributor Author

It seems some of the tests are running with /std:c++latest and using the deprecated member functions.
I'd like your input on what to do regarding these tests.

I have two options in mind:

  • Run these tests using /std:c++17 flag.
  • Adjust them to use std::allocator_trait if _HAS_CXX20.

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Jan 24, 2021
@StephanTLavavej
Copy link
Member

It seems some of the tests are running with /std:c++latest and using the deprecated member functions.
I'd like your input on what to do regarding these tests.

What we usually do is:

  • If the test doesn't "need" to use the deprecated machinery and can be changed to use modern machinery unconditionally, do so.
    • std::allocator_traits is always available, so this would make sense for a test that wasn't specifically concerned with exercising old allocator members.
  • If the test doesn't "need" to use the deprecated machinery, but the modern machinery only exists in newer language modes, it would be reasonable to #if _HAS_CXXversion in the test, if it isn't too much trouble.
    • Could apply in a hypothetical world where we deprecated/removed std::remove_if in favor of std::erase_if, not that that will ever happen.
  • If the test "needs" to use the deprecated machinery (i.e. it is specifically testing it), define the restore/warning-suppression macros in that test.
    • For example, tr1/tests/memory1 restores shared_ptr::unique and disables its deprecation:
      #define _HAS_DEPRECATED_SHARED_PTR_UNIQUE 1
      #define _SILENCE_CXX17_SHARED_PTR_UNIQUE_DEPRECATION_WARNING

In general, deprecated/removed machinery shouldn't cause us to stop running a test in /std:c++latest mode (the option you mentioned of "Run these tests using /std:c++17 flag."), as that would limit our test coverage. The exception would be if a test is only for deprecated/removed machinery. In that case, it would probably be easiest to just guard the entire test to do nothing in newer language modes, as we don't have "matrix" files that exercise only older language modes (whereas we do have matrices for newer language modes, like usual_latest_matrix.lst).

Looking at the very first test failure, for example:

void test_allocator_construct_const() {
// Annex D actually requires the default allocator to const_cast here
// See N4659 D.9 [depr.default.allocator]/6
int example = 0;
const int* const exampleCptr = &example;
allocator<int> alloc;
alloc.construct(&example, 42);
assert(example == 42);
allocator_traits<allocator<int>>::construct(alloc, exampleCptr, 1729);
assert(example == 1729);
}

Here, the test is exercising both deprecated machinery (the offending alloc.construct call) and modern machinery (the allocator_traits<allocator<int>>::construct call), looking for a specific issue (as opposed to using deprecated machinery to achieve something else). We've already suppressed the deprecation warning at the top, so it would be simplest to also define the
"restore machinery" macro. (Alternatively, just the lines testing deprecated machinery could be guarded with #if !_HAS_CXX20, which would be more localized in the sense that the rest of the test couldn't accidentally use the deprecated/removed tech; it's a judgement call and I'd be fine with either, really.)

@miscco
Copy link
Contributor

miscco commented Jan 24, 2021

I would like to note that @BillyONeal had some reservations about removing allocator<void>.

That is why I dropped it from the patch

@MichaelRizkalla
Copy link
Contributor Author

@StephanTLavavej I decided to define the restore machinery macro because the three failing tests are testing the removed functions as it seems and to be consistent with the other tests in the system. The PR should be ready for review now 😄.

@MichaelRizkalla
Copy link
Contributor Author

@miscco The current working draft and WG21-P0619 do not include the std::allocator<void> specialization therefore I removed it along with std::allocator's deprecated members. Also, I had a look at GH-380 now and I agree that std::allocator<void>'s presence is unobservable. I can drop that part from the PR if it's the better option 👀.

@BillyONeal
Copy link
Member

BillyONeal commented Jan 25, 2021

@miscco The current working draft and WG21-P0619 do not include the std::allocator<void> specialization therefore I removed it along with std::allocator's deprecated members. Also, I had a look at GH-380 now and I agree that std::allocator<void>'s presence is unobservable. I can drop that part from the PR if it's the better option 👀.

allocator<void> was removed from the WP because it needed to exist due to ill-formed-for-void things in the primary template, which were removed from the WP. However, we still provide those removed things because we support older language modes, so we still need an extra specialization so that allocator<void> can be named and all its members are accessible.

Base automatically changed from master to main January 28, 2021 00:35
@StephanTLavavej StephanTLavavej self-assigned this Jan 31, 2021
@StephanTLavavej StephanTLavavej merged commit d1f6fe9 into microsoft:main Feb 2, 2021
@StephanTLavavej
Copy link
Member

Thanks for encouraging code modernization! This will ship in VS 2019 16.10 Preview 1. 🚀 😺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<xmemory>: some std::allocator member functions still need to be removed in C++20
5 participants