[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17
Arthur O'Dwyer via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 3 12:55:54 PDT 2018
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 other nodiscard-related reviews, @EricWF suggested that we should warn all the time, even e.g. on a discarded `std::move(x)` in C++11, or a discarded `vec.empty()` in C++03. And *maybe* we could provide an opt-out mechanism, but honestly *I* don't see why anyone would want to opt out.
`_LIBCPP_FORCE_NODISCARD` smells like an opt-in mechanism, which I would think is the opposite of what we want.
================
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
----------------
What is the purpose of `_LIBCPP_DISABLE_NODISCARD_AFTER_CXX17`? I guess I could understand a blanket opt-in "please don't warn me about discarded [[nodiscard]] results"; but that should be (and is) spelled `-Wno-unused-result`, and it has nothing to do with C++17.
I like how this patch defines `_LIBCPP_NODISCARD` in non-C++17 modes; that's going to be very useful. But I think all these opt-in mechanisms are confusing and not-helpful.
If we must have an opt-in/out mechanism (which I don't believe we do), please consider adding the following two lines to `<__config>` and removing the rest:
#ifdef _LIBCPP_NODISCARD
// the user has given us their preferred spelling; use it unconditionally
#elif __has_cpp_attribute(nodiscard) && _LIBCPP_STD_VER > 17
[... etc etc ...]
Repository:
rCXX libc++
https://reviews.llvm.org/D45179
More information about the cfe-commits
mailing list