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

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius added a comment.

In D100311#2683984 <https://reviews.llvm.org/D100311#2683984>, @Quuxplusone wrote:

> FWIW, LGTM at this point except that I still think I'd prefer to see this tested in test/libcxx/diagnostics/nodiscard_extensions.verify.cpp.

Even if it's not an extension?



================
Comment at: libcxx/include/__node_handle:46
+//   explicit operator bool() const noexcept;
+//   [[nodiscard]] bool empty() const noexcept;
+//
----------------
Quuxplusone wrote:
> // nodiscard since C++20
> Also, shouldn't this whole comment block use `/* */` comments? Seeing nested `// //` comments is a bit weird. Unless we use `//` comments on some other synopsis comments already, perhaps?
Done and used `/* */`.


================
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:
> `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; }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100311