This is an archive of the discontinued LLVM Phabricator instance.

Differential D129054

[libc++] Consolidate the different [[nodiscard]] configuration options into a single one
ClosedPublic

Authored by philnik on Jul 3 2022, 5:52 PM.

Details

Reviewers
ldionne
Mordante
var-const
Group Reviewers
Restricted Project
Commits
rGb978dfbf749f: [libc++] Consolidate the different [[nodiscard]] configuration options into a…

Diff Detail

Unit TestsFailed

TimeTest
40 mslibcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx/diagnostics::enable_nodiscard.verify.cpp
40 mslibcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx/diagnostics::enable_nodiscard_disable_after_cxx17.verify.cpp
50 mslibcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx/diagnostics::enable_nodiscard_disable_nodiscard_ext.verify.cpp
780 mslibcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx/diagnostics::nodiscard_extensions.verify.cpp
830 mslibcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx/thread/thread_lock/thread_lock_guard::nodiscard.verify.cpp
View Full Test Results (16 Failed)

Event Timeline

philnik created this revision.Jul 3 2022, 5:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2022, 5:52 PM
philnik requested review of this revision.Jul 3 2022, 5:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2022, 5:52 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: libcxx-commits. · View Herald Transcript
Harbormaster completed remote builds in B173477: Diff 441987.Jul 3 2022, 6:16 PM
Mordante added a comment.Jul 5 2022, 9:57 AM

I wonder whether we want to take this approach. We've had patches in the past to remove this feature and make all [[nodiscard]] extensions always enabled. MSVC STL already does this and there have been done some tests on Google and Apple codebases. (I don't recall whether the results have been posted.)

How about going in that direction instead? probably we should then do this shortly after LLVM 15 is branched.

libcxx/docs/ReleaseNotes.rst
166

I feel this wording can use a bit of polish. I have a hard time to understand what this means.

libcxx/include/__config
833

Do we need two different macros or can they be combined? It doesn't need to be done in this patch.

philnik added a comment.Jul 5 2022, 12:57 PM
In D129054#3630743, @Mordante wrote:

I wonder whether we want to take this approach. We've had patches in the past to remove this feature and make all [[nodiscard]] extensions always enabled. MSVC STL already does this and there have been done some tests on Google and Apple codebases. (I don't recall whether the results have been posted.)

How about going in that direction instead? probably we should then do this shortly after LLVM 15 is branched.

That's what I want to do in D128267, but there @ldionne requested that the different configuration macros should be consolidated into one, which I'm doing in this patch. I think these are two separate issues, so I wanted to land them separately.

Mordante added a comment.Jul 7 2022, 9:37 AM
In D129054#3631115, @philnik wrote:
In D129054#3630743, @Mordante wrote:

I wonder whether we want to take this approach. We've had patches in the past to remove this feature and make all [[nodiscard]] extensions always enabled. MSVC STL already does this and there have been done some tests on Google and Apple codebases. (I don't recall whether the results have been posted.)

How about going in that direction instead? probably we should then do this shortly after LLVM 15 is branched.

That's what I want to do in D128267, but there @ldionne requested that the different configuration macros should be consolidated into one, which I'm doing in this patch. I think these are two separate issues, so I wanted to land them separately.

I missed that patch, thanks for the information. In that case consider my comment above as resolved.

ldionne requested changes to this revision.Aug 4 2022, 9:03 AM

This change will require some updates to the documentation, please grep for _LIBCPP_ENABLE_NODISCARD and _LIBCPP_DISABLE_NODISCARD_EXT.

I think we are on the same page in essence, but this patch does not 100% reflect that. I think we should remove all of our various nodiscard-related settings in favour of a single _LIBCPP_DISABLE_NODISCARD_EXT, which would be enabled by default. Then, in D128267, we can flip the default to _LIBCPP_DISABLE_NODISCARD_EXT being disabled by default (i.e. nodiscard extensions being enabled by default).

libcxx/include/__config
826
This revision now requires changes to proceed.Aug 4 2022, 9:03 AM
philnik updated this revision to Diff 452497.Aug 14 2022, 2:14 AM
philnik marked an inline comment as done.
  • Address comments
libcxx/include/__config
833

IIUC we still want to distinguish between standards-mandated applications and extensions.

Harbormaster completed remote builds in B181151: Diff 452497.Aug 14 2022, 2:55 AM
ldionne accepted this revision.Aug 18 2022, 8:58 AM

This LGTM, but can you please ping me on Discord for a quick look once CI is passing?

IMO this is a nice simplification, and I can hardly imagine users wanting to apply some extended applications of nodiscard, but not all of them. So the flexibility we provided didn't pull its weight, IMO.

This revision is now accepted and ready to land.Aug 18 2022, 8:58 AM
philnik updated this revision to Diff 453958.Aug 19 2022, 4:52 AM
  • Try to fix CI
Harbormaster completed remote builds in B182193: Diff 453958.Aug 19 2022, 5:41 AM
philnik updated this revision to Diff 453979.Aug 19 2022, 6:42 AM
  • Fix CI
Harbormaster completed remote builds in B182208: Diff 453979.Aug 19 2022, 8:10 AM
ldionne accepted this revision.Aug 25 2022, 8:06 AM
Closed by commit rGb978dfbf749f: [libc++] Consolidate the different [[nodiscard]] configuration options into a… (authored by philnik). · Explain WhyAug 25 2022, 1:01 PM
This revision was automatically updated to reflect the committed changes.
philnik added a commit: rGb978dfbf749f: [libc++] Consolidate the different [[nodiscard]] configuration options into a….
thakis added a subscriber: thakis.Aug 31 2022, 12:30 PM

We (used to) use _LIBCPP_ENABLE_NODISCARD. I only happened to see this change here by coincidence.

It would be nice if there was a

#ifdef _LIBCPP_ENABLE_NODISCARD
#error _LIBCPP_ENABLE_NODISCARD no longer has an effect, define _LIBCPP_ENABLE_NODISCARD_EXT
#endif

in some central header to inform people in a similar situation.

thakis added a comment.Aug 31 2022, 12:31 PM

(Also, libcxx/test/libcxx/diagnostics/nodiscard.pass.cpp still mentions _LIBCPP_ENABLE_NODISCARD in a comment.)

philnik added a comment.Aug 31 2022, 1:04 PM

@thakis I think your comments should be addressed by D128267.

thakis added a comment.Sep 1 2022, 7:25 AM
In D129054#3762104, @philnik wrote:

@thakis I think your comments should be addressed by D128267.

That's cool, but there still was (is) a Window where [[nodiscard]] disappeared for people who define _LIBCPP_ENABLE_NODISCARD.

In practice, it looks like nothing regressed for us in the 1 week it took us to notice this, but if some hard-to-update dependency had added code that ignored a nodiscard, this would've been pretty painful to deal with.

So I still think it'd be nice if that #error was there until D128267 lands.

Revision Contents

PathSize
libcxx/
docs/
5 lines
include/
4 lines

Diff 441987

libcxx/docs/ReleaseNotes.rst

Show First 20 Lines • Show All 154 Lines • ▼ Show 20 Lines- ``unary_function`` and ``binary_function`` are no longer available in C++17 and C++20.
They are also marked as ``[[deprecated]]`` in C++11 and later. To disable deprecation warnings They are also marked as ``[[deprecated]]`` in C++11 and later. To disable deprecation warnings
you have to define ``_LIBCPP_DISABLE_DEPRECATION_WARNINGS``. Note that this disables you have to define ``_LIBCPP_DISABLE_DEPRECATION_WARNINGS``. Note that this disables
all deprecation warnings. all deprecation warnings.
- The contents of ``<codecvt>``, ``wstring_convert`` and ``wbuffer_convert`` have been marked as deprecated. - The contents of ``<codecvt>``, ``wstring_convert`` and ``wbuffer_convert`` have been marked as deprecated.
To disable deprecation warnings you have to define ``_LIBCPP_DISABLE_DEPRECATION_WARNINGS``. Note that this To disable deprecation warnings you have to define ``_LIBCPP_DISABLE_DEPRECATION_WARNINGS``. Note that this
disables all deprecation warnings. disables all deprecation warnings.
- ``_LIBCPP_ENABLE_NODISCARD`` and ``_LIBCPP_DISABLE_NODISCARD_AFTER_CXX17`` are no longer respected.
Any standards-required ``[[nodiscard]]`` applications in C++20 are now always enabled. Any extended applications
can now be enabled by defining ``_LIBCPP_ENABLE_NODISCARD_EXT``. Note that this also enables applications of
``[[nodiscard]]`` in places where no version of the standard requires the attribute.
MordanteUnsubmitted
Not Done
ReplyInline Actions

I feel this wording can use a bit of polish. I have a hard time to understand what this means.

Mordante: I feel this wording can use a bit of polish. I have a hard time to understand what this means.
ABI Changes ABI Changes
----------- -----------
- The ``_LIBCPP_ABI_USE_CXX03_NULLPTR_EMULATION`` macro controlling whether we use an - The ``_LIBCPP_ABI_USE_CXX03_NULLPTR_EMULATION`` macro controlling whether we use an
emulation for ``std::nullptr_t`` in C++03 mode has been removed. After this change, emulation for ``std::nullptr_t`` in C++03 mode has been removed. After this change,
``_LIBCPP_ABI_USE_CXX03_NULLPTR_EMULATION`` will not be honoured anymore and there ``_LIBCPP_ABI_USE_CXX03_NULLPTR_EMULATION`` will not be honoured anymore and there
will be no way to opt back into the C++03 emulation of ``std::nullptr_t``. will be no way to opt back into the C++03 emulation of ``std::nullptr_t``.
▲ Show 20 Lines • Show All 56 Lines • Show Last 20 Lines

libcxx/include/__config

Show First 20 Lines • Show All 817 Lines • ▼ Show 20 Lines
// We can't use GCC's [[gnu::warn_unused_result]] and // We can't use GCC's [[gnu::warn_unused_result]] and
// __attribute__((warn_unused_result)), because GCC does not silence them via // __attribute__((warn_unused_result)), because GCC does not silence them via
// (void) cast. // (void) cast.
# define _LIBCPP_NODISCARD # define _LIBCPP_NODISCARD
# endif # endif
// _LIBCPP_NODISCARD_EXT may be used to apply [[nodiscard]] to entities not // _LIBCPP_NODISCARD_EXT may be used to apply [[nodiscard]] to entities not
// specified as such as an extension. // specified as such as an extension.
# if defined(_LIBCPP_ENABLE_NODISCARD) && !defined(_LIBCPP_DISABLE_NODISCARD_EXT) # if defined (_LIBCPP_ENABLE_NODISCARD_EXT)
ldionneUnsubmitted
Done
ReplyInline Actions
// specified as such as an extension.
- # if defined (_LIBCPP_ENABLE_NODISCARD_EXT)
+ # if defined(_LIBCPP_ENABLE_NODISCARD_EXT)
# define _LIBCPP_NODISCARD_EXT _LIBCPP_NODISCARD
ldionne:
# define _LIBCPP_NODISCARD_EXT _LIBCPP_NODISCARD # define _LIBCPP_NODISCARD_EXT _LIBCPP_NODISCARD
# else # else
# define _LIBCPP_NODISCARD_EXT # define _LIBCPP_NODISCARD_EXT
# endif # endif
# if !defined(_LIBCPP_DISABLE_NODISCARD_AFTER_CXX17) && (_LIBCPP_STD_VER > 17 || defined(_LIBCPP_ENABLE_NODISCARD)) # if _LIBCPP_STD_VER > 17 || defined(_LIBCPP_ENABLE_NODISCARD_EXT)
# define _LIBCPP_NODISCARD_AFTER_CXX17 _LIBCPP_NODISCARD # define _LIBCPP_NODISCARD_AFTER_CXX17 _LIBCPP_NODISCARD
MordanteUnsubmitted
Not Done
ReplyInline Actions

Do we need two different macros or can they be combined? It doesn't need to be done in this patch.

Mordante: Do we need two different macros or can they be combined? It doesn't need to be done in this…
philnikAuthorUnsubmitted
Done
ReplyInline Actions

IIUC we still want to distinguish between standards-mandated applications and extensions.

philnik: IIUC we still want to distinguish between standards-mandated applications and extensions.
# else # else
# define _LIBCPP_NODISCARD_AFTER_CXX17 # define _LIBCPP_NODISCARD_AFTER_CXX17
# endif # endif
# if __has_attribute(no_destroy) # if __has_attribute(no_destroy)
# define _LIBCPP_NO_DESTROY __attribute__((__no_destroy__)) # define _LIBCPP_NO_DESTROY __attribute__((__no_destroy__))
# else # else
# define _LIBCPP_NO_DESTROY # define _LIBCPP_NO_DESTROY
▲ Show 20 Lines • Show All 344 Lines • Show Last 20 Lines