Skip to content

P0439R0 enum class memory_order - #124

Merged
Stephan T. Lavavej (StephanTLavavej) merged 11 commits into
microsoft:masterfrom
AFFogarty:feature/P0439R0
Sep 27, 2019
Merged

P0439R0 enum class memory_order#124
Stephan T. Lavavej (StephanTLavavej) merged 11 commits into
microsoft:masterfrom
AFFogarty:feature/P0439R0

Conversation

@AFFogarty

@AFFogarty Andrew Fogarty (AFFogarty) commented Sep 22, 2019

Copy link
Copy Markdown
Contributor

Description

This PR implements #17. memory_order becomes an enum class in cxx20.

Checklist:

  • I understand README.md.
  • If this is a feature addition, that feature has been voted into the C++
    Working Draft.
  • Identifiers in any product code changes are properly _Ugly as per
    https://eel.is/c++draft/lex.name#3.1 .
  • The STL builds and test harnesses have passed (must be manually verified
    by an STL maintainer before CI is online, leave this unchecked for initial
    submission). (Internal PR 204684)
  • This change introduces no known ABI breaks (adding members, renaming
    members, adding virtual functions, changing whether a type is an aggregate or
    trivially copyable, etc.). If unsure, leave this box unchecked and ask a
    maintainer for help.

@stevenhoving

Copy link
Copy Markdown

Just my 2 cents: it might be a good idea to squash your commits into a single commit.

@sylveon

Copy link
Copy Markdown
Contributor

They can be squashed at merge time

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This feature needs to be mentioned in yvals_core.h in sorted order as being directly controlled by _HAS_CXX20:

// _HAS_CXX20 directly controls:
Note that we use the GitHub issue title (which you used in your PR title, thanks!) for these comments, not the original paper title if it differs.

I think that we can manually test and merge this feature, since the test impact should be very small to nonexistent. Thank you!

Comment thread stl/inc/atomic Outdated
Comment thread stl/inc/xatomic.h Outdated
Comment thread stl/inc/atomic
Comment thread stl/inc/atomic Outdated
Comment thread stl/inc/xatomic.h Outdated
Comment thread stl/inc/xatomic.h Outdated
Comment thread stl/inc/atomic Outdated
@AFFogarty Andrew Fogarty (AFFogarty) changed the title WIP: P0439R0 enum class memory_order P0439R0 enum class memory_order Sep 23, 2019

@BillyONeal Billy O'Neal (BillyONeal) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we want testing in place before merging this. I know the change is minor, and I think we could run the testing on your behalf, but I think we at least need a place for the tests to live first.

Comment thread stl/inc/atomic Outdated
Comment thread stl/inc/xatomic.h Outdated
@AFFogarty
Andrew Fogarty (AFFogarty) requested a review from a team as a code owner September 24, 2019 02:12
@AFFogarty

Copy link
Copy Markdown
Contributor Author

CC: Terry Kim (@imback82)

Comment thread stl/inc/atomic Outdated
Comment thread stl/inc/atomic Outdated
@CaseyCarter

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@BillyONeal

Copy link
Copy Markdown
Member

Casey Carter (@CaseyCarter) #114 needs to merge before that build will succeed.

@BillyONeal

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@StephanTLavavej

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Pull request contains merge conflicts.

@StephanTLavavej

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@BillyONeal Billy O'Neal (BillyONeal) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for your contribution!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is fine modulo test coverage.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Finishing an internal build/test before I merge this.

Comment thread stl/inc/atomic
@StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) merged commit 3ed27b9 into microsoft:master Sep 27, 2019
@AFFogarty

Copy link
Copy Markdown
Contributor Author

Thanks folks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants