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

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius 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
+
----------------
Quuxplusone wrote:
> 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.
Actually, I'm inclined to add this as an extension in another patch. First, I'd like to add missing tests for [[ http://wg21.link/P0600 | P0600 ]].


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100311