This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add [[nodiscard]] extensions to the functions in <bit>
ClosedPublic

Authored by philnik on Jun 11 2023, 11:54 AM.

Diff Detail

Event Timeline

philnik created this revision.Jun 11 2023, 11:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2023, 11:54 AM
philnik requested review of this revision.Jun 11 2023, 11:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2023, 11:54 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Jun 12 2023, 10:27 AM
ldionne added a subscriber: ldionne.
ldionne added inline comments.
libcxx/include/__bit/bit_ceil.h
20

Can you update the list in libcxx/docs/UsingLibcxx.rst?

This revision is now accepted and ready to land.Jun 12 2023, 10:27 AM
Mordante requested changes to this revision.Jun 12 2023, 10:33 AM
Mordante added a subscriber: Mordante.

Thanks for working on this!

libcxx/include/__bit/bit_floor.h
26

These should be documented in libcxx/docs/UsingLibcxx.rst.

Maybe we should look at a script/clang-tidy check to generate the documentation for these extensions.

libcxx/include/__bit/countr.h
26–34

These would make sense to, doesn't even need to be an extension.

libcxx/include/__bit/rotate.h
37

These should be nodiscard unconditionally. http://eel.is/c++draft/bit#rotate ...

I've no clue what makes these two functions special.

libcxx/test/libcxx/diagnostics/bit.nodiscard_extensions.compile.pass.cpp
20

Please add bit_cast too; it's already marked, but not tested.

This revision now requires changes to proceed.Jun 12 2023, 10:33 AM

I see Louis and I reviewed at the same time.

philnik updated this revision to Diff 530608.Jun 12 2023, 11:14 AM
philnik marked 5 inline comments as done.

Address comments

libcxx/include/__bit/bit_floor.h
26

IMO we should just remove the list, since I don't really see any use for it. A user doesn't care which functions are marked [[nodiscard]] unless the compiler complains (or doesn't complain if a [[nodiscard]] is missing), and implementers can just look at the code or the *.nodiscard_externsions.*.cpp. files or grep for _LIBCPP_NODISCARD_EXT.

Mordante accepted this revision.Jun 12 2023, 12:03 PM

LGTM modulo one nit.

libcxx/docs/UsingLibcxx.rst
407–412

FYI I landed D152097 today, this fixed the ordering of this list. So there might be some merge conflicts.

449–450

These are not extensions

libcxx/include/__bit/bit_floor.h
26

I strongly disagree. This is an extension and it should be documented.
Actually I think it would be good to see whether it would be good to Standardize noexcept in more places in the Standard so we don't need extensions.

This revision is now accepted and ready to land.Jun 12 2023, 12:03 PM
philnik marked an inline comment as done.Jun 12 2023, 12:20 PM
philnik added inline comments.
libcxx/include/__bit/bit_floor.h
26

Arguably it's not. Compilers are allowed to warn on anything they want, and that's what we do here - just in the library instead of the compiler. Just because we use a standard attribute for it doesn't mean it's an extension. It's not like there is a meaningful difference between [[nodiscard]] and [[clang:: warn_unused_result]]. Regarding standardization - IMO that's wasted time. Implementations can add it at their leisure, which makes spending committee time on it basically useless. Furthermore, implementations don't even have to add it, since it's just recommended practice to issue a diagnostic: http://eel.is/c++draft/dcl.attr.nodiscard#4, making every [[nodiscard]] in the standard nothing more than loose guidance.

This revision was automatically updated to reflect the committed changes.
philnik marked 3 inline comments as done.