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

[[nodiscard]] messages #2211

Merged
merged 85 commits into from
Aug 31, 2022
Merged

[[nodiscard]] messages #2211

merged 85 commits into from
Aug 31, 2022

Conversation

AlexGuteniev
Copy link
Contributor

@AlexGuteniev AlexGuteniev commented Sep 18, 2021

Fixes #271
Fixes #1742

@CaseyCarter CaseyCarter added the documentation Related to documentation or comments label Sep 18, 2021
@fsb4000
Copy link
Contributor

fsb4000 commented Sep 18, 2021

You can also add: "Fixes #1742"

Oops, I was wrong:

#elif __has_cpp_attribute(nodiscard) >= 201907L

@AlexGuteniev
Copy link
Contributor Author

Oops, I was wrong

Why, it still fixes #1742 where possible

@fsb4000
Copy link
Contributor

fsb4000 commented Sep 18, 2021

Why, it still fixes #1742 where possible

You are correct.
I tested on godbolt: https://godbolt.org/z/r1fPM3b9K
So __has_cpp_attribute(nodiscard) >= 201907L is true for /std:c++14

stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/xmemory Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/future Show resolved Hide resolved
stl/inc/memory Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Aug 28, 2022
@StephanTLavavej
Copy link
Member

Thanks for this major diagnostic improvement 😻 - and apologies for taking so very long to review this! I've pushed a bunch of changes, renaming/rephrasing stuff and updating a few more locations to use the message macros. Please let me know if anything looks undesirable. (I was probably most aggressive with rephrasing the assume_aligned message; I thought it was better to follow your launder pattern and not describe in too much detail how the compiler may or may not optimize.)

@AlexGuteniev
Copy link
Contributor Author

Please let me know if anything looks undesirable.

I intentionally did not place full stop at the end of each message for consistency with other ranges. Consider this program:

[[nodiscard("don't discard. the value is useful")]]
bool lock()
{
    return false;
}

int main()
{
    int i;
    lock();
}

The warnings (/W4):

ConsoleApplication14.cpp(19,9): warning C4858: discarding return value: don't discard. the value is useful
ConsoleApplication14.cpp(18,9): warning C4101: 'i': unreferenced local variable

Do you still want to have it?

@AlexGuteniev

This comment was marked as resolved.

@StephanTLavavej
Copy link
Member

(capturing Discord discussion here for the record)

The compiler definitely prefers short phrases without periods for most of its warning messages. However, longer messages (typically static analysis warnings) are sometimes full sentences with periods, like https://godbolt.org/z/v9ansc3fa :

<source>(3) : warning C28251: Inconsistent annotation for 'new': this instance has no annotations.
See c:\users\containeradministrator\appdata\local\temp\compiler-explorer-compiler2022728-2624-10iodxs.ufwa\predefined c++ types (compiler internal)(33).
The first user-provided annotation on this built-in function is at line c:\data\msvc\14.33.31629\include\vcruntime_new.h(48). 

And our [[deprecated("message")]] explanations, which are the closest precedent for [[nodiscard("message")]], have periods:

STL/stl/inc/yvals_core.h

Lines 935 to 939 in 8ca7bd3

[[deprecated("warning STL4014: " \
"std::result_of and std::result_of_t are deprecated in C++17. " \
"They are superseded by std::invoke_result and std::invoke_result_t. " \
"You can define _SILENCE_CXX17_RESULT_OF_DEPRECATION_WARNING " \
"or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to suppress this warning.")]]

Thus while there are arguments for both ways, we've concluded that keeping the periods would be best.

@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit 539c26c into microsoft:main Aug 31, 2022
@StephanTLavavej
Copy link
Member

Thanks again for enhancing the STL's favorite warning! 😻 ⚠️ 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Related to documentation or comments
Projects
None yet
6 participants