[libc++] Consolidate the different [[nodiscard]] configuration options into a single one
Needs RevisionPublic
Actions

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

Details

Reviewers

ldionne
Mordante
var-const

Group Reviewers

Restricted Project

Diff Detail

Repository: rG LLVM Github Monorepo

Unit TestsFailed
View All

	Time	Test
	40 ms	libcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx/diagnostics::enable_nodiscard.verify.cpp Script: -- : 'COMPILED WITH'; /usr/bin/c++ /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/libcxx/test/libcxx/diagnostics/enable_nodiscard.verify.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++11 -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -D_LIBCPP_ENABLE_NODISCARD -fsyntax-only -Wno-error -Xclang -verify -Xclang -verify-ignore-unexpected=note -ferror-limit=0
	40 ms	libcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx/diagnostics::enable_nodiscard_disable_after_cxx17.verify.cpp Script: -- : 'COMPILED WITH'; /usr/bin/c++ /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/libcxx/test/libcxx/diagnostics/enable_nodiscard_disable_after_cxx17.verify.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++11 -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -D_LIBCPP_ENABLE_NODISCARD -D_LIBCPP_DISABLE_NODISCARD_AFTER_CXX17 -fsyntax-only -Wno-error -Xclang -verify -Xclang -verify-ignore-unexpected=note -ferror-limit=0
	50 ms	libcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx/diagnostics::enable_nodiscard_disable_nodiscard_ext.verify.cpp Script: -- : 'COMPILED WITH'; /usr/bin/c++ /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/libcxx/test/libcxx/diagnostics/enable_nodiscard_disable_nodiscard_ext.verify.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++11 -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -D_LIBCPP_ENABLE_NODISCARD -D_LIBCPP_DISABLE_NODISCARD_EXT -fsyntax-only -Wno-error -Xclang -verify -Xclang -verify-ignore-unexpected=note -ferror-limit=0
	780 ms	libcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx/diagnostics::nodiscard_extensions.verify.cpp Script: -- : 'COMPILED WITH'; /usr/bin/c++ /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/libcxx/test/libcxx/diagnostics/nodiscard_extensions.verify.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++11 -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -D_LIBCPP_ENABLE_NODISCARD -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -fsyntax-only -Wno-error -Xclang -verify -Xclang -verify-ignore-unexpected=note -ferror-limit=0
	830 ms	libcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx/thread/thread_lock/thread_lock_guard::nodiscard.verify.cpp Script: -- : 'COMPILED WITH'; /usr/bin/c++ /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/libcxx/test/libcxx/thread/thread.lock/thread.lock.guard/nodiscard.verify.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/983e0059110f-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++11 -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -D_LIBCPP_ENABLE_NODISCARD -fsyntax-only -Wno-error -Xclang -verify -Xclang -verify-ignore-unexpected=note -ferror-limit=0
		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

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.

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.

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.

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.Thu, Aug 4, 9:03 AM

Revision Contents
Changeset List

Path

Size

libcxx/

docs/

ReleaseNotes.rst

5 lines

include/

__config

4 lines

Diff 441987

View Options

libcxx/docs/ReleaseNotes.rst

View Options

[libc++] Consolidate the different [[nodiscard]] configuration options into a single oneNeeds RevisionPublicActions