Use consistent phrasing of required standard mode #252

SuperWig · 2019-11-03T11:30:51Z

Description

Hopefully resolves #250 as it seemed like you were all in agreement.

Used the phrase "The contents of <meow> are ~~only available~~ available only with C++meow or later."
Changed the #else comments to use the name of the macro
Inverted the condition so the message is at the top of the file.
Removed a newline in <variant> as none of the other headers had one there.

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 and
the C++ Working Draft as a reference (and any other cited standards).
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.

stl/inc/any

stl/inc/concepts

stl/inc/ranges

stl/inc/concepts

SuperWig · 2019-11-04T13:10:41Z

stl/inc/ranges

@@ -34,8 +36,6 @@ _STD_END
 _STL_RESTORE_CLANG_WARNINGS
 #pragma warning(pop)
 #pragma pack(pop)
-#else // ^^^ Concepts support / no Concepts vvv
-#pragma message("The contents of <ranges> are only available with C++20 concepts support.")
 #endif // __cpp_lib_concepts


Should these be defined(__cpp_lib_concepts)?

Good question. Our current practice is inconsistent (because we've added arrow comments over the years). Your PR currently follows one of our patterns - repeating the macro name as the #endif comment, despite the arrow comment on the #else. So this would be fine to leave as-is. You could also consistently use a matching arrow, which would be // ^^^ defined(__cpp_lib_concepts) ^^^ here. (The one thing we don't do is pair #ifndef MEOW with #endif // defined(MEOW).)

Well I assume they would eventually be replaced with _HAS_CXX20 at some point. So I guess It would eventually solve itself?

StephanTLavavej

I think these changes are great. I'm inclined to merge them as-is, and if we want to make our preprocessor comments more consistent, do that in a separate PR.

StephanTLavavej · 2019-11-04T19:47:00Z

stl/inc/ranges

@@ -34,8 +36,6 @@ _STD_END
 _STL_RESTORE_CLANG_WARNINGS
 #pragma warning(pop)
 #pragma pack(pop)
-#else // ^^^ Concepts support / no Concepts vvv
-#pragma message("The contents of <ranges> are only available with C++20 concepts support.")
 #endif // __cpp_lib_concepts


Good question. Our current practice is inconsistent (because we've added arrow comments over the years). Your PR currently follows one of our patterns - repeating the macro name as the #endif comment, despite the arrow comment on the #else. So this would be fine to leave as-is. You could also consistently use a matching arrow, which would be // ^^^ defined(__cpp_lib_concepts) ^^^ here. (The one thing we don't do is pair #ifndef MEOW with #endif // defined(MEOW).)

StephanTLavavej · 2019-11-04T19:54:15Z

Note that when merged, #133, #142, and #223 will all need to follow this pattern.

StephanTLavavej · 2019-11-04T19:54:22Z

/azp run

azure-pipelines · 2019-11-04T19:54:30Z

Azure Pipelines successfully started running 1 pipeline(s).

StephanTLavavej · 2019-11-04T20:00:26Z

<execution> and <variant> are failing the clang-format step (possibly due to trailing whitespace? clang-format validation runs before the whitespace checker.)

BillyONeal · 2019-11-04T20:07:51Z

I wish I knew how to make the PR bot just fix it rather than complaining.

I could do 'git diff' rather than 'git status' to print out differences though?

SuperWig · 2019-11-04T20:34:45Z

They seem to be in places unrelated to this PR 😕

StephanTLavavej · 2019-11-04T22:26:31Z

/azp run

azure-pipelines · 2019-11-04T22:26:40Z

Azure Pipelines successfully started running 1 pipeline(s).

StephanTLavavej · 2019-11-04T22:33:09Z

I believe clang-format's behavior is affected by the phrasing of preprocessor conditionals, so it decided to format different branches. (I don't understand its criteria yet.)

StephanTLavavej · 2019-11-04T22:46:44Z

I wish I knew how to make the PR bot just fix it rather than complaining.

I don't believe that's supported.

I could do 'git diff' rather than 'git status' to print out differences though?

I created #255 for this.

BillyONeal · 2019-11-05T03:33:43Z

stl/inc/any

@@ -9,7 +9,9 @@
 #include <yvals_core.h>
 #if _STL_COMPILER_PREPROCESSOR

-#if _HAS_CXX17
+#if !_HAS_CXX17


I am usually the person who hates inverted conditionals with else, but I think this is the exception that proves the rule :) Thanks!

StephanTLavavej · 2019-11-05T21:13:48Z

Thanks for the improvement! Hopefully this will help some users understand why they're getting these diagnostics (especially with the message now at the top of each header).

use consistent phrasing of required standard mode

67f1f4d

SuperWig requested a review from a team as a code owner November 3, 2019 11:30

CaseyCarter requested changes Nov 3, 2019

View reviewed changes

stl/inc/any Outdated Show resolved Hide resolved

stl/inc/concepts Outdated Show resolved Hide resolved

stl/inc/ranges Outdated Show resolved Hide resolved

stl/inc/concepts Outdated Show resolved Hide resolved

SuperWig added 3 commits November 3, 2019 16:36

Revert <concepts> and <ranges> wording

bbdbbce

!defined()/defined()

9be0d9d

only available => available only

6f0d907

SuperWig force-pushed the mode branch from a6aa49f to 6f0d907 Compare November 3, 2019 17:59

SuperWig requested a review from CaseyCarter November 4, 2019 07:55

SuperWig commented Nov 4, 2019

View reviewed changes

StephanTLavavej approved these changes Nov 4, 2019

View reviewed changes

whitespace?

9efc99c

StephanTLavavej approved these changes Nov 5, 2019

View reviewed changes

StephanTLavavej requested review from barcharcraz and BillyONeal November 5, 2019 00:06

barcharcraz approved these changes Nov 5, 2019

View reviewed changes

BillyONeal approved these changes Nov 5, 2019

View reviewed changes

miscco mentioned this pull request Nov 5, 2019

<span> Implement span #142

Merged

7 tasks

CaseyCarter approved these changes Nov 5, 2019

View reviewed changes

StephanTLavavej merged commit 9d635aa into microsoft:master Nov 5, 2019

SuperWig deleted the mode branch November 6, 2019 16:04

miscco mentioned this pull request Nov 20, 2019

<stop_token> and jthread - WIP #223

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use consistent phrasing of required standard mode #252

Use consistent phrasing of required standard mode #252

SuperWig commented Nov 3, 2019 •

edited

Loading

SuperWig Nov 4, 2019

StephanTLavavej Nov 4, 2019

SuperWig Nov 4, 2019

StephanTLavavej left a comment

StephanTLavavej Nov 4, 2019

StephanTLavavej commented Nov 4, 2019

StephanTLavavej commented Nov 4, 2019

azure-pipelines bot commented Nov 4, 2019

StephanTLavavej commented Nov 4, 2019

BillyONeal commented Nov 4, 2019

SuperWig commented Nov 4, 2019 •

edited

Loading

StephanTLavavej commented Nov 4, 2019

azure-pipelines bot commented Nov 4, 2019

StephanTLavavej commented Nov 4, 2019

StephanTLavavej commented Nov 4, 2019

BillyONeal Nov 5, 2019

StephanTLavavej commented Nov 5, 2019

Use consistent phrasing of required standard mode #252

Use consistent phrasing of required standard mode #252

Conversation

SuperWig commented Nov 3, 2019 • edited Loading

Description

Checklist

SuperWig Nov 4, 2019

Choose a reason for hiding this comment

StephanTLavavej Nov 4, 2019

Choose a reason for hiding this comment

SuperWig Nov 4, 2019

Choose a reason for hiding this comment

StephanTLavavej left a comment

Choose a reason for hiding this comment

StephanTLavavej Nov 4, 2019

Choose a reason for hiding this comment

StephanTLavavej commented Nov 4, 2019

StephanTLavavej commented Nov 4, 2019

azure-pipelines bot commented Nov 4, 2019

StephanTLavavej commented Nov 4, 2019

BillyONeal commented Nov 4, 2019

SuperWig commented Nov 4, 2019 • edited Loading

StephanTLavavej commented Nov 4, 2019

azure-pipelines bot commented Nov 4, 2019

StephanTLavavej commented Nov 4, 2019

StephanTLavavej commented Nov 4, 2019

BillyONeal Nov 5, 2019

Choose a reason for hiding this comment

StephanTLavavej commented Nov 5, 2019

SuperWig commented Nov 3, 2019 •

edited

Loading

SuperWig commented Nov 4, 2019 •

edited

Loading