Skip to content

Conversation

@celonymire
Copy link
Contributor

@celonymire celonymire commented Jan 17, 2021

Further addresses #206.

Even though it's deprecated, it's still in the codebase, and I've found some member functions that seemingly can be marked as _NODISCARD.

Also, I'm very unsure about the helper functions, so if I missed anything in there that can be marked _NODISCARD or has to be removed somewhere, then please let me know.

@celonymire celonymire requested a review from a team as a code owner January 17, 2021 19:21
Copy link
Contributor

@miscco miscco left a comment

Choose a reason for hiding this comment

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

Looks reasonable, however, these files will be removed with the vNext rbanch https://github.com/microsoft/STL/wiki/vNext-Planning

@celonymire
Copy link
Contributor Author

Oh well 😄

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Jan 18, 2021
@StephanTLavavej
Copy link
Member

While we are indeed planning to remove <hash_meow> in vNext, I think we can review and merge this PR - _NODISCARD is low risk and easy to review.

Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! I'll go ahead and push changes for the two issues (one stylistic) that I found.

@StephanTLavavej StephanTLavavej self-assigned this Jan 26, 2021
@StephanTLavavej StephanTLavavej merged commit 0efeeb4 into microsoft:master Jan 26, 2021
@StephanTLavavej
Copy link
Member

Thanks for adding even more occurrences of the best attribute ever to the STL! All shall love [[nodiscard]] and despair! 😻

@celonymire celonymire deleted the even_more_nodiscard branch January 26, 2021 14:59
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.

5 participants