Skip to content

P0439R0 enum class memory_order #124

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

Merged
merged 11 commits into from
Sep 27, 2019

Conversation

AFFogarty
Copy link
Contributor

@AFFogarty AFFogarty commented Sep 22, 2019

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

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

@sylveon
Copy link
Contributor

sylveon commented Sep 22, 2019

They can be squashed at merge time

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.

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!

@AFFogarty AFFogarty changed the title WIP: P0439R0 enum class memory_order P0439R0 enum class memory_order Sep 23, 2019
Copy link
Member

@BillyONeal BillyONeal left a comment

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.

@AFFogarty AFFogarty requested a review from a team as a code owner September 24, 2019 02:12
@AFFogarty
Copy link
Contributor Author

CC: @imback82

@CaseyCarter
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BillyONeal
Copy link
Member

@CaseyCarter #114 needs to merge before that build will succeed.

@BillyONeal
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephanTLavavej
Copy link
Member

/azp run

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@StephanTLavavej
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@BillyONeal BillyONeal left a comment

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

This is fine modulo test coverage.

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.

Finishing an internal build/test before I merge this.

@StephanTLavavej StephanTLavavej merged commit 3ed27b9 into microsoft:master Sep 27, 2019
@AFFogarty
Copy link
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