This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement D2351R0 "Mark all library static cast wrappers as [[nodiscard]]"
ClosedPublic

Authored by Quuxplusone on Apr 5 2021, 12:02 PM.

Details

Summary
These [[nodiscard]] annotations are added as a conforming extension;
it's unclear whether the paper will actually be adopted and make them
mandatory, but they do seem like good ideas regardless.

https://isocpp.org/files/papers/D2351R0.pdf

This patch implements the paper's effect on:
- std::to_integer, std::to_underlying
- std::forward, std::move, std::move_if_noexcept
- std::as_const
- std::identity

The paper also affects (but libc++ does not yet have an implementation of):
- std::bit_cast

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Apr 5 2021, 12:02 PM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptApr 5 2021, 12:02 PM
cor3ntin accepted this revision.Apr 5 2021, 12:52 PM
curdeius accepted this revision as: curdeius.Apr 5 2021, 1:33 PM

LGTM. It can indeed be seen as a QoI change.

ldionne requested changes to this revision.Apr 5 2021, 1:52 PM

Can we add libc++ specific tests for those? A simple .verify.cpp test for each should be fairly easy to add.

This revision now requires changes to proceed.Apr 5 2021, 1:52 PM

@ldionne: Good point. It turns out that there's a dedicated place to hang these new test cases! (Easily found by grepping libcxx/test/ for "nodiscard", once you'd pointed it out.)

I decided to go ahead and mention std::bit_cast in the test file, under a code comment, so that maybe whoever adds bit_cast will be grepping for it and find these tests and update them.

ldionne accepted this revision.Apr 5 2021, 2:41 PM

@ldionne: Good point. It turns out that there's a dedicated place to hang these new test cases! (Easily found by grepping libcxx/test/ for "nodiscard", once you'd pointed it out.)

I decided to go ahead and mention std::bit_cast in the test file, under a code comment, so that maybe whoever adds bit_cast will be grepping for it and find these tests and update them.

Re: bit_cast, my implementation was stuck on compiler bugs last time I checked. See D75960.

This revision is now accepted and ready to land.Apr 5 2021, 2:41 PM

Fix forward.fail.cpp, which checks the wording of the static_assert "can not" typo that I fixed incidentally.
Poke buildkite.