[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/include/__node_handle:16
+// public:
+//   // These type declarations are described in Tables 82 and 83.
+//   using value_type     = see below;     // not present for map containers
----------------
Nit: suggest removing this comment line, as it'll just bit-rot.


================
Comment at: libcxx/include/__node_handle:46
+//   explicit operator bool() const noexcept;
+//   [[nodiscard]] bool empty() const noexcept;
+//
----------------
// 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?


================
Comment at: libcxx/test/std/containers/container.node/node_handle.empty.nodiscard.verify.cpp:16
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
----------------
`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.


================
Comment at: libcxx/test/std/containers/container.node/node_handle.empty.nodiscard.verify.cpp:25
+void test() {
+  Container c;
+  auto n = c.extract(c.begin());
----------------
I suggest populating this container, to avoid the appearance of trying to extract from an empty container. (Even if this test //is// compile-only, which I think it is, despite the `main` routine.)
I suggest:
```
template<class Container>
void test_one(Container c) {
    auto n = c.extract(c.begin());
    n.empty();  // expected-warning 8{{ignoring return value of function}}
}

void test() {
    test_one(std::set<int>{1});
    test_one(std::multiset<int>{1});
    test_one(std::map<int, int>{{1, 1}});
    test_one(std::multimap<int, int>{{1, 1}});
    test_one(std::unordered_set<int>{1});
    test_one(std::unordered_multiset<int>{1});
    test_one(std::unordered_map<int, int>{{1, 1}});
    test_one(std::unordered_multimap<int, int>{{1, 1}});
}
```
(and no `main` routine).

However, see my other comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100311