Skip to content
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

yvals_core.h: Consistent diagnostics and warnings #2973

Merged

Conversation

sam20908
Copy link
Contributor

Fixes #237
Fixed broken PR history of #2969

@sam20908 sam20908 requested a review from a team as a code owner July 30, 2022 13:53
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Jul 30, 2022
@StephanTLavavej StephanTLavavej added this to Initial Review in Code Reviews Jul 30, 2022
@strega-nil-ms strega-nil-ms self-assigned this Aug 2, 2022
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

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

stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Show resolved Hide resolved
Code Reviews automation moved this from Initial Review to Work In Progress Aug 3, 2022
@strega-nil-ms strega-nil-ms removed their assignment Aug 3, 2022
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej moved this from Work In Progress to Final Review in Code Reviews Aug 4, 2022
@StephanTLavavej StephanTLavavej added the decision needed We need to choose something before working on this label Aug 5, 2022
@StephanTLavavej
Copy link
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 <yvals_core.h>. If we decide that this should be changed, the occurrences in <hash_map>, <hash_set>, <type_traits>, <experimental/coroutine>, and <experimental/filesystem> should be checked.

@StephanTLavavej StephanTLavavej removed the decision needed We need to choose something before working on this label Aug 10, 2022
@StephanTLavavej
Copy link
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).

@StephanTLavavej StephanTLavavej self-assigned this Aug 10, 2022
sam20908 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
Member

Thanks! Everything in <yvals_core.h> 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 @strega-nil-ms as I pushed these changes after you approved (I believe they all align with what you wanted).

@StephanTLavavej StephanTLavavej removed their assignment Aug 11, 2022
@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Aug 11, 2022
@StephanTLavavej StephanTLavavej self-assigned this Aug 11, 2022
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit 9475fa6 into microsoft:main Aug 12, 2022
Code Reviews automation moved this from Ready To Merge to Done Aug 12, 2022
@StephanTLavavej
Copy link
Member

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

@sam20908 sam20908 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
No open projects
Development

Successfully merging this pull request may close these issues.

<yvals_core.h>: "acknowledge this warning" vs. "suppress this deprecation"
4 participants