Skip to content

Conversation

@Arzaghi
Copy link
Contributor

@Arzaghi Arzaghi commented Oct 8, 2020

Fixes #1037

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Oct 8, 2020
@miscco
Copy link
Contributor

miscco commented Oct 9, 2020

From what I read in the bug description we should also fiy the move assignment.

@Arzaghi Arzaghi marked this pull request as ready for review October 11, 2020 10:10
@Arzaghi Arzaghi requested a review from a team as a code owner October 11, 2020 10:10
In addition to being conventional, _STD expands to ::std:: which avoids
any possible ambiguity.
* It's okay for container assignment to require comparator assignment.

* Destroy-and-reconstruct is exception-unsafe - if reconstructing the
comparator throws an exception (e.g. because the comparator attempts to
allocate memory), then the container will be left with a destroyed
comparator, violating its invariants.

* It's intentional that container move construction/assignment performs
comparator copy construction/assignment, respectively. The Standard
(annoyingly!) doesn't talk about this clearly, but it's necessary to
preserve container invariants. The issue is that moved-from containers
still need to be in a valid-but-unspecified state (i.e. preconditionless
member functions can be called), so they can be clear()ed (which is
preconditionless) followed by inserting elements. This requires their
comparators to still be usable. However, a moved-from comparator may not
be usable - in particular, moved-from std::functions may be empty (and
are, in our implementation). Therefore, because comparators must always
be usable by the associative containers, they can't be moved-from.

This was unclear from the way I wrote up microsoftGH-1037; there I said
"`_Move_assign` also copy-assigns the comparator, then calls `_Swap_val`
anyways." and I should have said that the former was intentional but the
latter was undesirable.
This swaps the comparator in a slightly different order,
but that's unobservable.
@StephanTLavavej
Copy link
Member

Thanks! I've pushed a batch of changes:

  • Fix typo: "constuctible" => "constructible"
  • Change std:: to _STD in product code.
    • In addition to being conventional, _STD expands to ::std:: which avoids any possible ambiguity.
    • Superseded by later changes.
  • In _Move_assign overloads, restore comparator copy assignment.
    • It's okay for container assignment to require comparator assignment.
    • Destroy-and-reconstruct is exception-unsafe - if reconstructing the comparator throws an exception (e.g. because the comparator attempts to allocate memory), then the container will be left with a destroyed comparator, violating its invariants.
    • It's intentional that container move construction/assignment performs comparator copy construction/assignment, respectively. The Standard (annoyingly!) doesn't talk about this clearly, but it's necessary to preserve container invariants. The issue is that moved-from containers still need to be in a valid-but-unspecified state (i.e. preconditionless member functions can be called), so they can be clear()ed (which is preconditionless) followed by inserting elements. This requires their comparators to still be usable. However, a moved-from comparator may not be usable - in particular, moved-from std::functions may be empty (and are, in our implementation). Therefore, because comparators must always be usable by the associative containers, they can't be moved-from.
    • This was unclear from the way I wrote up <xtree>: _Tree move constructor incorrectly swaps #1037; there I said "_Move_assign also copy-assigns the comparator, then calls _Swap_val anyways." and I should have said that the former was intentional but the latter was undesirable.
  • Centralize code into _Swap_val_excluding_comp.
    • This code was somewhat lengthy and repeated 3 times, making it worth extracting into a helper function.
  • Centralize code even more by removing _Swap_val.
    • This swaps the comparator in a slightly different order, but that's unobservable.
  • Cleanup: Add top-level const to _Right_scary (a pointer).
  • Comment when we're intentionally copying comparators.

@StephanTLavavej StephanTLavavej removed their assignment Oct 28, 2020
@CaseyCarter CaseyCarter self-assigned this Oct 28, 2020
@StephanTLavavej StephanTLavavej self-assigned this Oct 29, 2020
@StephanTLavavej StephanTLavavej merged commit f6c8d33 into microsoft:master Oct 29, 2020
@StephanTLavavej
Copy link
Member

Thanks for fixing this longstanding bug (present since at least VS 2015 and possibly all the way back to VS 2010) and improving the usability of the associative containers! 😺

@Arzaghi Arzaghi deleted the Fix_Issue1037 branch October 29, 2020 07:50
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.

<xtree>: _Tree move constructor incorrectly swaps

4 participants