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

<syncstream>: implement basic_syncbuf and basic_osyncstream #1564

Merged
merged 62 commits into from
Feb 19, 2021
Merged

<syncstream>: implement basic_syncbuf and basic_osyncstream #1564

merged 62 commits into from
Feb 19, 2021

Conversation

MichaelRizkalla
Copy link
Contributor

@MichaelRizkalla MichaelRizkalla commented Jan 11, 2021

This PR will implement P0053R7 and P0753R2. should close #3.

It's still on going work, current progress tracking:

  • Implement basic_syncbuf
  • Implement basic_osyncstream
  • Implement osyncstream manipulators
  • Implement tests for basic_syncbuf
  • Implement tests for basic_osyncstream
  • Implement tests for osyncstream manipulators

@MichaelRizkalla MichaelRizkalla requested a review from a team as a code owner January 11, 2021 14:58
stl/inc/syncstream Outdated Show resolved Hide resolved
@MichaelRizkalla

This comment has been minimized.

stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
Copy link
Contributor Author

@MichaelRizkalla MichaelRizkalla left a comment

Choose a reason for hiding this comment

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

I started this review to have all questions that I think need a maintainer's input.

stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/src/syncstream.cpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added the cxx20 C++20 feature label Jan 12, 2021
@StephanTLavavej

This comment has been minimized.

stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/src/syncstream.cpp Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/src/syncstream.cpp Outdated Show resolved Hide resolved
stl/src/syncstream.cpp Outdated Show resolved Hide resolved
stl/src/syncstream.cpp Outdated Show resolved Hide resolved
stl/inc/iosfwd Show resolved Hide resolved
Copy link
Contributor

@miscco miscco left a comment

Choose a reason for hiding this comment

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

There are some concerns about fancy pointers and allocators, but besides that mostly nits.

Great progress 👍

stl/src/syncstream.cpp Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/src/syncstream.cpp Outdated Show resolved Hide resolved
stl/src/syncstream.cpp Outdated Show resolved Hide resolved
stl/src/syncstream.cpp Outdated Show resolved Hide resolved
stl/src/syncstream.cpp Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
Copy link
Contributor Author

@MichaelRizkalla MichaelRizkalla left a comment

Choose a reason for hiding this comment

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

I started sorting out the tests for this PR.
I do not really understand why P1502R1_standard_library_header_units is failing now, so I'd like your input.

@MichaelRizkalla

This comment has been minimized.

@crackedmind

This comment has been minimized.

MichaelRizkalla and others added 5 commits January 18, 2021 14:30
- Re-structure _Basic_sync_global_mutex
- Re-organize functions in basic_syncbuf
- Update related files when adding a new header
Co-Authored-By: Adam Bucior <[email protected]>
- it does not protect against overflow, but for test purposes it should be (fine?)

Co-Authored-By: Stephan T. Lavavej <[email protected]>
@StephanTLavavej
Copy link
Member

Thanks for fixing the fancy pointer and allocator issues! I've pushed two commits, adding explicit to the ID class's constructor (it's test code, but I always recommend being cautious about implicitly converting constructors), and adding the necessary stl/msbuild changes. I've mirrored this to internal MSVC-PR-303125 along with the other necessary internal changes. 😸 (This is mirrored all by itself, not chained with other GitHub PRs, due to the magnitude of the changes and the importance of the feature.)

Please note: Further changes are still possible (e.g. if you notice something, or if final review leads to more feedback that should be addressed before merging), but they must be replicated to the mirror PR in order to avoid codebase divergence.

@StephanTLavavej

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

This comment is increasingly confusing - it was added when the
BuildFiles below was moved to the top, which is the important part.
special_math.cpp doesn't need to be literally first.

We aren't going to arbitrarily reorder these lists, and in the long
term, our CMake build system will replace our legacy msbuild system.
@StephanTLavavej
Copy link
Member

I've pushed a comment change to an internal stl/msbuild file (technically unrelated to this PR, but right above a modification, and @CaseyCarter noticed that the comment was confusing).

@StephanTLavavej
Copy link
Member

I've pushed and mirrored a change to use _Init_locks, which resolves the Standard Library Header Units crash (so we can restore that part of the test). There's a Static Initialization Order Fiasco, which was revealed but not inherently caused by modules, where the syncstream map requires the STL's locks to be initialized (because for IDL=2, we need to take the debug lock in map's destructor to orphan all of its iterators). If the locks aren't guaranteed to be initialized before map construction, then they aren't guaranteed to be destroyed after map destruction, and we're doomed.

The fix is to use the _Init_locks code pattern that's used elsewhere in the STL:

STL/stl/src/cout.cpp

Lines 9 to 11 in 8f79acf

#pragma warning(disable : 4074)
#pragma init_seg(compiler)
static std::_Init_locks initlocks;

This guarantees initialization order within the syncstream.cpp translation unit. (I had seen this pattern before, but forgot about it since I've never added occurrences in my 14 years working on the STL - first time for everything.)

I have copied the exact spelling of the _Init_locks pattern for searchability, even though we should probably say _STD for consistency (that should be a global cleanup). The non-_Ugly identifier initlocks is fine because this is static (again, we could clean it up, but that would be a separate global cleanup).

Copy link
Contributor

@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.

Just a couple of noexcept questions, looks good otherwise.

stl/inc/syncstream Outdated Show resolved Hide resolved
stl/inc/syncstream Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter removed their assignment Feb 19, 2021
@StephanTLavavej StephanTLavavej merged commit 834baa6 into microsoft:main Feb 19, 2021
@StephanTLavavej
Copy link
Member

Thanks for implementing this new stream! 🎉 🚀 😺 This will ship in VS 2019 16.10 Preview 2.

@MichaelRizkalla MichaelRizkalla deleted the syncstream branch February 20, 2021 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx20 C++20 feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P0053R7 <syncstream>
10 participants