Skip to content

Conversation

@Arzaghi
Copy link
Contributor

@Arzaghi Arzaghi commented Sep 30, 2020

Fixes #1324

@Arzaghi Arzaghi marked this pull request as ready for review September 30, 2020 23:22
@Arzaghi Arzaghi requested a review from a team as a code owner September 30, 2020 23:22
@StephanTLavavej StephanTLavavej added the performance Must go faster label Oct 1, 2020
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Oct 2, 2020

I pushed a fix for the test failure that I caused by getting greedy with the pointer() => nullptr cleanup. It revealed a pre-existing bug in the old (VS 2010-era) Dev10_722102_shared_ptr_nullptr test. That contained a fancy_pointer being given to unique_ptr which totally failed to meet the Cpp17NullablePointer requirements. Fixes:

  • Include <cstddef> for nullptr_t (the test is already using namespace std;) and change the other headers for consistency.
  • Cpp17NullablePointer requires implicit construction from nullptr, which previously wouldn't compile. Add a constructor and a data member initializer T* ptr_{nullptr};.
  • We also need to provide assignment from nullptr - I can never remember whether this will invoke implicit construction and whether that's hazardous in corner cases, so I added an assignment operator for simplicity.
  • We also need to be equality/inequality comparable, with other fancy pointers and nullptr. Provide all forms (not bothering to drop any for C++20 mode).

With this, the test should pass, and I believe it meets the requirements now.

@CaseyCarter
Copy link
Contributor

CaseyCarter commented Oct 2, 2020

  • Cpp17NullablePointer requires value-initialized pointers to be null, but instead this was garbage. Fix this with a data member initializer: T* ptr_{nullptr};

Value-init of this class previously resulted in zero-initialization since the default constructor was not user-provided. (I don't know if there's a name for this "trivial default-init but deterministic value-init" idiom.)

  • We also need to provide assignment from nullptr - I can never remember whether this will invoke implicit construction and whether that's hazardous in corner cases, so I added an assignment operator for simplicity.

Implicit construction, yes, and the most common failure is trying to assign convertible to convertible to X, violating the "at most one user-defined conversion" rule.

@StephanTLavavej
Copy link
Member

Value-init of this class previously resulted in zero-initialization since the default constructor was not user-provided.

Ah, you are indeed correct. I updated my description accordingly, thanks. (Yay, the code was only broken in N - 1 ways.)

@StephanTLavavej StephanTLavavej self-assigned this Oct 2, 2020
@StephanTLavavej StephanTLavavej merged commit 02130c4 into microsoft:master Oct 3, 2020
@StephanTLavavej
Copy link
Member

Thanks for improving vector performance - the STL's most important data structure! This should have the bonus effect of increasing compiler throughput (as it was reported to us by @xiangfan-ms on the compiler front-end team). 🚀 🚀

@Arzaghi Arzaghi deleted the Fix_Issue1324 branch October 3, 2020 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance Must go faster

Projects

None yet

Development

Successfully merging this pull request may close these issues.

<vector>: vector's move ctor initializes members twice

5 participants