Skip to content

Replace __pragma with _Pragma#662

Merged
barcharcraz merged 3 commits intomicrosoft:masterfrom
CdTCzech:#659
Apr 2, 2020
Merged

Replace __pragma with _Pragma#662
barcharcraz merged 3 commits intomicrosoft:masterfrom
CdTCzech:#659

Conversation

@CdTCzech
Copy link
Contributor

@CdTCzech CdTCzech commented Mar 31, 2020

Description

Resolves #659

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

  • Identifiers in product code changes are properly _Ugly as per
    https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
  • The STL builds successfully and all tests have passed (must be manually
    verified by an STL maintainer before automated testing is enabled on GitHub,
    leave this unchecked for initial submission).
  • These changes introduce no known ABI breaks (adding members, renaming
    members, adding virtual functions, changing whether a type is an aggregate
    or trivially copyable, etc.).
  • These changes were written from scratch using only this repository,
    the C++ Working Draft (including any cited standards), other WG21 papers
    (excluding reference implementations outside of proposed standard wording),
    and LWG issues as reference material. If they were derived from a project
    that's already listed in NOTICE.txt, that's fine, but please mention it.
    If they were derived from any other project (including Boost and libc++,
    which are not yet listed in NOTICE.txt), you must mention it here,
    so we can determine whether the license is compatible and what else needs
    to be done.

@CdTCzech
Replace __pragma with _Pragma
d7960f4 
VS 2019 16.6 Preview 2 now supports Standard _Pragma.
@CdTCzech CdTCzech requested a review from a team as a code owner March 31, 2020 12:19
@miscco
Copy link
Contributor

miscco commented Mar 31, 2020

From the comment in the issue:

Note: Standard _Pragma takes a string while non-Standard __pragma doesn't, as the existing code demonstrates.

@CdTCzech
Copy link
Contributor Author

CdTCzech commented Mar 31, 2020 •
edited
Loading

From the comment in the issue:

Note: Standard _Pragma takes a string while non-Standard __pragma doesn't, as the existing code demonstrates.

Oh, such a mistake :( I was so scared of my first pull request ever that I forgot to sync my changes properly...
EDIT: Fixed

@CaseyCarter
Copy link
Contributor

CaseyCarter commented Mar 31, 2020

I was so scared of my first pull request ever that I forgot to sync my changes properly...

No need to be scared: we save our soul-crushing toxicity for the second pull request. (Don't worry about the test failures; tr1/fstream1 is broken due to a race condition that is being fixed in #647.)

@CaseyCarter CaseyCarter added the enhancement Something can be improved label Mar 31, 2020
@cbezault
Copy link
Contributor

cbezault commented Mar 31, 2020

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 31, 2020

Azure Pipelines successfully started running 1 pipeline(s).

@barcharcraz barcharcraz merged commit 42d5df0 into microsoft:master Apr 2, 2020
@barcharcraz
Copy link
Contributor

barcharcraz commented Apr 2, 2020

Thank you for the contribution!

fengjixuchui added a commit to fengjixuchui/STL that referenced this pull request Apr 2, 2020
@fengjixuchui
Merge pull request #50 from microsoft/master
f2d927c 
Replace __pragma with _Pragma (microsoft#662)
barcharcraz pushed a commit to barcharcraz/STL that referenced this pull request Apr 2, 2020
Revert "Replace __pragma with _Pragma (microsoft#662)"
2c2dc29 
This reverts commit 42d5df0.

Reverting due to a bug in how /Zc:preprocessor handles _Pragma
see VSO-1091758 for more details
@barcharcraz
Copy link
Contributor

barcharcraz commented Apr 2, 2020

Unfortunately we had to revert this PR due to issues with _Pragma in 16.6 preview 2 under /Zc:preprocessor. :(

We'll revert the PR once that issue is fixed

barcharcraz pushed a commit that referenced this pull request Apr 2, 2020
Revert "Replace __pragma with _Pragma (#662)" (#675)
3e8b269 
This reverts commit 42d5df0.

Reverting due to a bug in how /Zc:preprocessor handles _Pragma
see VSO-1091758 for more details

Co-authored-by: Charles <barto.charlie+code@gmail.com>
@CdTCzech
Copy link
Contributor Author

CdTCzech commented Apr 2, 2020

Unfortunately we had to revert this PR due to issues with _Pragma in 16.6 preview 2 under /Zc:preprocessor. :(

We'll revert the PR once that issue is fixed

I'm always glad to be a first revert of the repository 😄
Thanks for letting me know and hopefully it will go smoother with my second one!

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Apr 2, 2020

Not a problem with your work at all! 😸 In fact, your change has led to two concrete improvements: (1) we've updated our checklist for merging PRs, and (2) we were completely lacking STL test coverage for /Zc:preprocessor, both on GitHub and in our Microsoft-internal test harness. The only thing that identified this was range-v3 test coverage in the compiler tests, that happened to drag in the STL (specifically <atomic>) and use /Zc:preprocessor. I had totally forgotten that we were missing any use of this option in tests/std - we're usually very diligent about testing conformance modes like /permissive- and so forth. @barcharcraz is now working on adding proper test coverage, so when the compiler bug is fixed and ships in a public Preview, we'll be able to restore your change with confidence.

@barcharcraz barcharcraz mentioned this pull request Apr 2, 2020
Merged
@CdTCzech CdTCzech deleted the #659 branch April 20, 2020 15:13
CdTCzech added a commit to CdTCzech/STL that referenced this pull request Oct 5, 2020
@CdTCzech
Replace __pragma with _Pragma
4adceda 
This change was already merged as microsoft#662, but later reverted in microsoft#675.
Resolves microsoft#659
@CdTCzech CdTCzech mentioned this pull request Oct 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+2 more reviewers

@CaseyCarter CaseyCarter CaseyCarter approved these changes

@BillyONeal BillyONeal BillyONeal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@barcharcraz barcharcraz

Labels

enhancement Something can be improved

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

<yvals_core.h>: Use Standard _Pragma

7 participants

@CdTCzech @miscco @CaseyCarter @cbezault @barcharcraz @StephanTLavavej @BillyONeal