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 `_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.
> _LIBCPP_FORCE_NODISCARD smells like an opt-in mechanism
Correct.
> And *maybe* we could provide an opt-out mechanism, but honestly *I* don't see
> why anyone would want to opt out.
I agree.
> which I would think is the opposite of what we want.
Also correct.
================
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
----------------
Quuxplusone wrote:
> 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 ...]
>
> 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.
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.
```
#ifdef _LIBCPP_NODISCARD
// the user has given us their preferred spelling; use it unconditionally
```
So you propose to shift the burden of picking which define to use to each and
every
libc++ user (who wants to enable nodiscard attribute for pre-C++17/whatever)
out there?
I really don't see how that would be better.
Repository:
rCXX libc++
https://reviews.llvm.org/D45179
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
