[libcxx-commits] [PATCH] D100311: [libc++] Test nodiscard attributes of node-handle::empty().

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/containers/container.node/node_handle.empty.nodiscard.verify.cpp:16
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
----------------
curdeius wrote:
> Quuxplusone wrote:
> > `extract` and `empty` were added in C++17, so we should have a C++17 test for this. It looks like we mark `empty` as nodiscard only "AFTER_CXX17"; we should also test that it's marked, as an extension, in C++17 mode. We have existing tests for nodiscard extensions in
> > test/libcxx/diagnostics/nodiscard_extensions.pass.cpp
> > test/libcxx/diagnostics/nodiscard_extensions.verify.cpp
> > and I'd like to see these tests added in there for consistency, not scattered here, if humanly possible.
> Not sure if I understand you correctly, you want to have all the tests there or only those for the C++17 extension?
> 
> Also, adding `[[nodiscard]]` as an extension would mean having this ugliness (or adding yet another macro):
> ```
>     #if _LIBCPP_STD_VER <= 17
>     _LIBCPP_NODISCARD_EXT
>     #endif
>     _LIBCPP_NODISCARD_AFTER_CXX17 _LIBCPP_INLINE_VISIBILITY
>     bool empty() const { return __ptr_ == nullptr; }
> ```
IIUC, if the user defines `-D_LIBCPP_ENABLE_NODISCARD=1`, then `_LIBCPP_NODISCARD_AFTER_CXX17` expands to `[[nodiscard]]` in all supported modes (yes even C++11/14/17).

Basically, `_LIBCPP_NODISCARD_AFTER_CXX17` already implies "WG21 has decided that making this [[nodiscard]] is a good idea," which implies "making this [[nodiscard]] is always a good idea." So it already implies the extension. We just need tests for the extension.

I had intended "all the tests there," but I'm not confident in that — I now think it's likely that your tests here should stay here, but the C++17 extension tests should go over there. You'll have to take a look at those files and see how they're currently organized.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100311/new/

https://reviews.llvm.org/D100311