Page MenuHomePhabricator

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

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

Details

Reviewers
ldionne
Mordante
var-const
Group Reviewers
Restricted Project

Diff Detail

Unit TestsFailed

TimeTest
40 mslibcxx 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 mslibcxx 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 mslibcxx 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 mslibcxx 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 mslibcxx 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

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.

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