[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

EricWF commandeered this revision. EricWF edited reviewers, added: lebedev.ri; removed: EricWF. EricWF added a comment. Hijacking with permission. Repository: rL LLVM https://reviews.llvm.org/D45179 ___ cfe-commits mailing list

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

lebedev.ri added a comment. Ping? Repository: rL LLVM https://reviews.llvm.org/D45179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

lebedev.ri updated this revision to Diff 152587. lebedev.ri marked an inline comment as done. lebedev.ri added a comment. Herald added a subscriber: llvm-commits. Right, sorry. Let's try to revive this. Does this test coverage look better? What did i miss? Repository: rL LLVM

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

thakis added a comment. What's the status here? The MS STL has this feature under _HAS_NODISCARD and it's super useful. Repository: rCXX libc++ https://reviews.llvm.org/D45179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

mclow.lists added a comment. BTW, you can gang several failing tests together, and check all the error messages - see libcxx/test/libcxx/containers/unord/unord.set/missing_hash_specialization.fail.cpp for an example. Repository: rCXX libc++ https://reviews.llvm.org/D45179

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

mclow.lists added inline comments. Comment at: test/libcxx/diagnostics/force_nodiscard.fail.cpp:22 + +_LIBCPP_NODISCARD_AFTER_CXX17 int foo() { return 6; } + lebedev.ri wrote: > mclow.lists wrote: > > Shouldn't this be just `_LIBCPP_NODISCARD` ? > > > I don't

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

lebedev.ri added inline comments. Comment at: test/libcxx/diagnostics/force_nodiscard.fail.cpp:22 + +_LIBCPP_NODISCARD_AFTER_CXX17 int foo() { return 6; } + mclow.lists wrote: > Shouldn't this be just `_LIBCPP_NODISCARD` ? > I don't think so? I thought we are

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

mclow.lists added inline comments. Comment at: test/libcxx/diagnostics/force_nodiscard.fail.cpp:22 + +_LIBCPP_NODISCARD_AFTER_CXX17 int foo() { return 6; } + Shouldn't this be just `_LIBCPP_NODISCARD` ? Repository: rCXX libc++

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

lebedev.ri updated this revision to Diff 143872. lebedev.ri marked 8 inline comments as done. lebedev.ri added a comment. Updated based on @mclow.lists review. Repository: rCXX libc++ https://reviews.llvm.org/D45179 Files: include/__config

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

lebedev.ri added inline comments. Comment at: test/libcxx/diagnostics/force_nodiscard.pass.cpp:16 +// MODULES_DEFINES: _LIBCPP_FORCE_NODISCARD +#define _LIBCPP_DISABLE_NODISCARD_AFTER_CXX17 +#define _LIBCPP_FORCE_NODISCARD lebedev.ri wrote: > Quuxplusone wrote:

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

lebedev.ri added inline comments. Comment at: include/__config:1016 +// because GCC does not silence them via (void) cast. +#if __has_cpp_attribute(nodiscard) && _LIBCPP_STD_VER > 17 +# define _LIBCPP_NODISCARD [[nodiscard]] mclow.lists wrote: > lebedev.ri

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

mclow.lists added inline comments. Comment at: include/__config:1016 +// because GCC does not silence them via (void) cast. +#if __has_cpp_attribute(nodiscard) && _LIBCPP_STD_VER > 17 +# define _LIBCPP_NODISCARD [[nodiscard]] lebedev.ri wrote: > mclow.lists

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

lebedev.ri added inline comments. Comment at: include/__config:1016 +// because GCC does not silence them via (void) cast. +#if __has_cpp_attribute(nodiscard) && _LIBCPP_STD_VER > 17 +# define _LIBCPP_NODISCARD [[nodiscard]] mclow.lists wrote: > `[[nodiscard]]`

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

mclow.lists added a comment. In https://reviews.llvm.org/D45179#1077055, @thakis wrote: > So you're happy with this opt-in version? I'm happy with an opt-in mechanism, yes. This one is not quite right yet. BTW, I expect a //large// set of calls in the standard library to get marked as

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

rjmccall added a comment. In https://reviews.llvm.org/D45179#1077048, @mclow.lists wrote: > In https://reviews.llvm.org/D45179#1056183, @rjmccall wrote: > > > Is Marshall arguing that the standard doesn't allow compilers to warn about > > failing to use these function results prior to C++17?

Re: [PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

So you're happy with this opt-in version? On Tue, Apr 24, 2018 at 1:29 PM, Marshall Clow via Phabricator via cfe-commits wrote: > mclow.lists added a comment. > > In https://reviews.llvm.org/D45179#1056183, @rjmccall wrote: > > > Is Marshall arguing that the standard

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

mclow.lists added a comment. In https://reviews.llvm.org/D45179#1056183, @rjmccall wrote: > Is Marshall arguing that the standard doesn't allow compilers to warn about > failing to use these function results prior to C++17? Because I don't think > that's true; warnings are thoroughly

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

Quuxplusone added a comment. In https://reviews.llvm.org/D45179#1064589, @lebedev.ri wrote: > I'm waiting for @mclow.lists to have the final say re this differential. Ack. :) > So roughly: > > // NOTE: Do not use [[nodiscard]] in pre-C++17 mode > // to avoid -Wc++17-extensions

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

lebedev.ri added a comment. I'm waiting for @mclow.lists to have the final say re this differential. In https://reviews.llvm.org/D45179#1057187, @Quuxplusone wrote: > In https://reviews.llvm.org/D45179#1056706, @lebedev.ri wrote: > > > gcc does not silence the warning produced by > > these

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

Quuxplusone added a comment. In https://reviews.llvm.org/D45179#1056706, @lebedev.ri wrote: > gcc does not silence the warning produced by > these attributes via `(void)` cast, unless it's `[[nodiscard]]` attribute :/ > [...] So until they fix that (they will fix that right? :)), > we can

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

lebedev.ri updated this revision to Diff 140934. lebedev.ri added a comment. Based on IRC disscussion, try to update the patch. The main problem is that gcc does not silence the warning produced by these attributes via `(void)` cast, unless it's `[[nodiscard]]` attribute :/

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

rjmccall added a comment. Yes, if we think that the committee is likely to include questionable functions on the `[[nodiscard]]` list which we don't want to warn about pre-C++17, then it makes sense to have two internal macros, one for functions like `std::move` that should be unconditionally

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

EricWF added a comment. I'm also in favor of making this opt-out instead of opt-in, except for a handful of functions like `unique_ptr::release` which may yield false positives. However, for functions where discarding the return value is always a bug, I don't see an issue in libc++ emitting a

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

lebedev.ri added a comment. In https://reviews.llvm.org/D45179#1056183, @rjmccall wrote: > Is Marshall arguing that the standard doesn't allow compilers to warn about > failing to use these function results prior to C++17? No. He was simply saying that people are opposed to having nodiscard

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

rjmccall added a comment. Is Marshall arguing that the standard doesn't allow compilers to warn about failing to use these function results prior to C++17? Because I don't think that's true; warnings are thoroughly non-normative. If libc++ wants to allow its users to opt out of these warnings

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

lebedev.ri added a comment. In https://reviews.llvm.org/D45179#1056108, @Quuxplusone wrote: > > Opt-in is an enhancement. Of course, it would be nice to always default it > > to on, but as it was disscussed with @mclow.lists, this is simply not going > > to happen. This is the best we'll get.

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

Quuxplusone added a comment. >> If we must have an opt-in/out mechanism (which I don't believe we do) > > Yes, we do. Opt-out is pre-existing, and removing it would be an > [unacceptable] regression. Ah, I see now that you weren't the originator of `_LIBCPP_DISABLE_NODISCARD_AFTER_CXX17`;

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

lebedev.ri added inline comments. Comment at: test/libcxx/diagnostics/force_nodiscard.fail.cpp:15 +// MODULES_DEFINES: _LIBCPP_FORCE_NODISCARD +#define _LIBCPP_FORCE_NODISCARD +#include <__config> Quuxplusone wrote: > What is the purpose of

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

Quuxplusone added inline comments. Comment at: test/libcxx/diagnostics/force_nodiscard.fail.cpp:15 +// MODULES_DEFINES: _LIBCPP_FORCE_NODISCARD +#define _LIBCPP_FORCE_NODISCARD +#include <__config> What is the purpose of `_LIBCPP_FORCE_NODISCARD`? On one of your