This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Enable [[nodiscard]] extensions by default
ClosedPublic

Authored by philnik on Jun 21 2022, 5:41 AM.

Details

Summary

Adding [[nodiscard]] to functions is a conforming extension and done extensively in the MSVC STL.

Diff Detail

Event Timeline

philnik created this revision.Jun 21 2022, 5:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2022, 5:41 AM
philnik requested review of this revision.Jun 21 2022, 5:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2022, 5:41 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
EricWF accepted this revision.Jun 21 2022, 8:14 AM

LGTM once we get this clean.

Thanks for providing a way to turn it off.

This revision is now accepted and ready to land.Jun 21 2022, 8:14 AM
jloser added a subscriber: jloser.Jun 21 2022, 8:38 AM
jloser added inline comments.
libcxx/docs/UsingLibcxx.rst
343–344

This needs to read "can be disabled by defining `_LIBCPP_DISABLE_NODISCARD`" (note disable, not enable in the macro identifier)

ldionne requested changes to this revision.Jun 23 2022, 11:21 AM

If we want to move forward with this patch, we're missing a bunch of changes. Please grep for _LIBCPP_ENABLE_NODISCARD in the codebase.

Once you've re-uploaded, I'll run a build internally to give us an idea of how the impact it might have on users. I suggest we discuss this once we have more data in hand.

BTW, if we find out that it doesn't break much code, and if we all agree that it's useful to catch bugs, then perhaps it doesn't make sense to provide an escape hatch at all.

This revision now requires changes to proceed.Jun 23 2022, 11:21 AM
philnik updated this revision to Diff 440058.Jun 26 2022, 6:35 AM
  • Address comments
ldionne added inline comments.Jun 30 2022, 5:55 AM
libcxx/include/__config
861

This whole thing is really confusing, and I think we need to clean it up. We have:

_LIBCPP_DISABLE_NODISCARD
_LIBCPP_DISABLE_NODISCARD_EXT
_LIBCPP_DISABLE_NODISCARD_AFTER_CXX17

I don't understand why we have all that. We should only have one knob: _LIBCPP_DISABLE_NODISCARD_EXT, which would disable the nodiscards that we added as an extension.

philnik updated this revision to Diff 455008.Aug 23 2022, 4:30 PM
philnik marked 2 inline comments as done.
  • Rebased
ldionne requested changes to this revision.Aug 25 2022, 9:42 AM
ldionne added inline comments.
libcxx/docs/UsingLibcxx.rst
342–343

Let's break the line here.

libcxx/include/__config
867

I think this is what we mean:

  1. In C++20, always mark those things required by the standard as nodiscard
  2. In < C++20, always mark as nodiscard unless the user has opted out by defining _LIBCPP_DISABLE_NODISCARD_EXT.
870

(not attached to any particular line)

I think we need to go through all of our tests for nodiscard. Many of them will currently be marked as // UNSUPPORTED: c++03, c++11, c++14. Instead, they should be changed to: // UNSUPPORTED: (c++03 || c++11 || c++14) && !stdlib=libc++ so that we gain coverage for this newly added behavior.

This revision now requires changes to proceed.Aug 25 2022, 9:42 AM
philnik updated this revision to Diff 456163.Aug 27 2022, 5:09 PM
philnik marked 3 inline comments as done.
  • Address comments
philnik updated this revision to Diff 457065.Aug 31 2022, 1:03 PM
  • Try to fix CI
philnik updated this revision to Diff 457214.Sep 1 2022, 3:03 AM
  • Next try
ldionne accepted this revision.Sep 2 2022, 9:38 AM
ldionne added a subscriber: cjdb.

@cjdb You finally had your way :-)

I think this is a good change, the only things this uncovered when I tested in an internal build were actual bugs.

This revision is now accepted and ready to land.Sep 2 2022, 9:38 AM
ldionne added a subscriber: Restricted Project.Sep 2 2022, 9:39 AM

Pinging vendors for awareness, since this may start breaking builds that compile with -Werror.

This revision was automatically updated to reflect the committed changes.
libcxx/include/__config