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

<expected>: Make copy/move assignment operators of expected propagate triviality #4271

Merged

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Dec 16, 2023

The conditions for triviality should be consistent with optional and variant (see WG21-P0602R4). I've tried to submit an LWG issue for this, but I believe we can conformingly do this without changing the standard wording. Edit: the issue is now LWG-4026.

The changes are also being made in libc++, see LLVM-74768.

Old test coverage was a bit crazy

More than 1000 function template specializations are instantiated. But I have no good idea to reduce the instantiation.

  • It was actually somehow bad, there're failures with "fatal error C1060: compiler is out of heap space".

Edit: reduced the cases from 32×32 to 6×6.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner December 16, 2023 18:02
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Dec 16, 2023
@StephanTLavavej
Copy link
Member

As I mentioned on Discord, the #ifdef EXHAUSTIVE technique seen in a few other tests would be applicable. I don't think we need the full 2N combinatoric explosion for normal automated testing. Instead, it should be sufficient to test that each individual condition can influence the outcome (e.g. a type where only the copy assignment is non-trivial).

Also, /d1reportMemorySummary can be used to check the compiler's peak working set size. We don't have a specific limit for this, but in my opinion any test that consumes more than 1 GB for either x86 or x64 is doing too much work. We have a number of ranges tests that exceed this and they're quite problematic.

@CaseyCarter
Copy link
Member

As I mentioned on Discord, the #ifdef EXHAUSTIVE technique seen in a few other tests would be applicable. I don't think we need the full 2N combinatoric explosion for normal automated testing. Instead, it should be sufficient to test that each individual condition can influence the outcome (e.g. a type where only the copy assignment is non-trivial).

In practice, there are (1) no types for which copy operations are trivial but moves are not, (2) no types for which assignment is trivial but construction is not, and (3) no types for which some copy or move operation is trivial but destruction is not. It's sufficient to test five cases:

  1. no trivial operations
  2. only destruction is trivial
  3. only destruction and move construct are trivial
  4. only destruction and move construct/assign are trivial
  5. only destruction and move/copy construct are trivial
  6. destruction and move/copy construct/assign are all trivial.
    Other cases only exist in test suites and it's a waste of time to specify (let alone test) their behavior.

stl/inc/expected Outdated Show resolved Hide resolved
tests/std/tests/P0323R12_expected/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Jan 20, 2024
@StephanTLavavej StephanTLavavej self-assigned this Jan 24, 2024
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit 065e5ff into microsoft:main Jan 25, 2024
35 checks passed
@StephanTLavavej
Copy link
Member

Thanks for this highly nontrivial enhancement! 😹 🎉 🚀

@frederick-vs-ja frederick-vs-ja deleted the trivially-assignable-expected branch January 25, 2024 00:59
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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants