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

Use _NODISCARD_FRIEND everywhere #2622

Merged
merged 1 commit into from
Apr 4, 2022

Conversation

StephanTLavavej
Copy link
Member

Thanks to @MattStephanson in #2208, we now have _NODISCARD_FRIEND, a centralized workaround for a bug that affects CUDA.

In theory, we could restrict usage of _NODISCARD_FRIEND to C++14 (and possibly C++17 - I'm not sure if the newer CUDA versions that accept C++17 are affected by this bug). However, we've already accumulated some usage in C++20 code:

_NODISCARD_FRIEND constexpr bool operator<(const _Base128& _Left, const _Base128& _Right) noexcept {

As the macro is exactly the same number of characters, it seems simpler to use it everywhere.

This should fix DevCom-1555597 / VSO-1426765 "VS2019 standard exception headers: warning C5240: 'nodiscard': attribute is ignored in this syntactic position".

Should fix DevCom-1555597 / VSO-1426765.
@StephanTLavavej StephanTLavavej added the bug Something isn't working label Mar 29, 2022
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner March 29, 2022 02:32
@AlexGuteniev
Copy link
Contributor

I don't like that nodiscard is combined with friend. There's nodiscard message PR where nodiscard is replaced with more specific. Having nodiscard friend macro may require more message-specific macros

@CaseyCarter
Copy link
Member

Having nodiscard friend macro may require more message-specific macros

_NODISCARD(message) and _NODISCARD_FRIEND(message) is two macros - doesn't seem too terrible to me.

Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I validated that there's now a single match for the regex _NODISCARD\s*friend in the STL, the expansion for the macro _NODISCARD_FRIEND.

@StephanTLavavej
Copy link
Member Author

I don't like that nodiscard is combined with friend.

I'm also not delighted that this workaround is necessary, but CUDA support is an important scenario, and (as Casey mentioned) while there will be merge conflicts with nodiscard-message, I don't believe this represents a true conflict - notably, there should be no need for two kinds of messages (friends vs. non-friends).

At some point in the future, we may be able to drop support for CUDA 10.1 Update 2, but that's a decision for our bosses and boss-like entities (my vague understanding is that users in the CUDA ecosystem upgrade a bit faster than one might expect, due to the need to support new GPUs, but I don't know any concrete percentages for usage).

This was referenced Mar 30, 2022
@StephanTLavavej StephanTLavavej self-assigned this Apr 1, 2022
@StephanTLavavej
Copy link
Member Author

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

@StephanTLavavej StephanTLavavej merged commit bcf2b79 into microsoft:main Apr 4, 2022
@StephanTLavavej StephanTLavavej deleted the nodiscard-friend branch April 4, 2022 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants