Skip to content

yvals_core.h: Consistent diagnostics and warnings - #2973

Merged
Stephan T. Lavavej (StephanTLavavej) merged 10 commits into
microsoft:mainfrom
celonymire:consistent_diagonstics_and_warnings
Aug 12, 2022
Merged

Stephan T. Lavavej (StephanTLavavej) merged 10 commits into
microsoft:mainfrom
celonymire:consistent_diagonstics_and_warnings

Conversation

@celonymire

Copy link
Copy Markdown
Contributor

Fixes #237
Fixed broken PR history of #2969

@celonymire
Sam Huang (celonymire) requested a review from a team as a code owner July 30, 2022 13:53
@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) added the enhancement Something can be improved label Jul 30, 2022

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.

You should also change anything that says "suppress this deprecation" to "suppress this warning".

Comment thread stl/inc/yvals_core.h Outdated
Comment thread stl/inc/yvals_core.h
@strega-nil-ms nicole mazzuca (strega-nil-ms) removed their assignment Aug 3, 2022
Comment thread stl/inc/yvals_core.h Outdated
Comment thread stl/inc/yvals_core.h Outdated
@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) added the decision needed We need to choose something before working on this label Aug 5, 2022
@StephanTLavavej

Copy link
Copy Markdown
Member

Marking as decision needed for the reasons that I explained here: #237 (comment) (I suspect I made that comment before we created the decision needed label.)

Also, there are occurrences of "acknowledge" outside . If we decide that this should be changed, the occurrences in , , , , and should be checked.

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) removed the decision needed We need to choose something before working on this label Aug 10, 2022
@StephanTLavavej

Copy link
Copy Markdown
Member

We talked about this at the weekly maintainer meeting - the consensus was that we should consistently say suppress, and that we should refer to things as warnings or errors but not diagnostics (as that's Standard jargon, not in wide use among programmers).

Sam Huang (celonymire) and others added 6 commits August 10, 2022 15:24
Drop "and acknowledge that this is unsupported"; this repeats "currently do not support Clang".
Unify "acknowledge that you understand this message and" and "silence this message and" into "To suppress this error,".

Say "confirm" and drop "actually".
@StephanTLavavej

Copy link
Copy Markdown
Member

Thanks! Everything in looks good. Searching for all occurrences of "acknowledge" and "diagnostic", I found a few more files that needed to be updated (I previously mentioned some of them):

  • Say "suppress this error" for "hard deprecations".
  • Say "suppress this error" for the Clang experimental/coroutine message.
    • Drop "and acknowledge that this is unsupported"; this repeats "currently do not support Clang".
  • Reword the aligned_storage message.
    • Unify "acknowledge that you understand this message and" and "silence this message and" into "To suppress this error,".
    • Say "confirm" and drop "actually".
  • Change "diagnostic" to "error" in the join_view message.

FYI nicole mazzuca (@strega-nil-ms) as I pushed these changes after you approved (I believe they all align with what you wanted).

@StephanTLavavej

Copy link
Copy Markdown
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) merged commit 9475fa6 into microsoft:main Aug 12, 2022
@StephanTLavavej

Copy link
Copy Markdown
Member

Thanks for making these messages simpler and more consistent! 💬 🎉 😸

@celonymire
Sam Huang (celonymire) deleted the consistent_diagonstics_and_warnings branch August 27, 2022 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Something can be improved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

: "acknowledge this warning" vs. "suppress this deprecation"

4 participants