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

[• Quuxplusone]

libcxx/include/__node_handle

16

Nit: suggest removing this comment line, as it'll just bit-rot.

46

// 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?

libcxx/test/std/containers/container.node/node_handle.empty.nodiscard.verify.cpp

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.

26

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.

[• Quuxplusone]

D99895 is related, btw (and I just landed it, so rebase before attempting to mess with libcxx/test/libcxx/diagnostics if you're going that route).

[curdeius]

• Apply review comments.

[• Quuxplusone]

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.

[curdeius]

In 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?

libcxx/include/__node_handle

46

Done and used /* */.

libcxx/test/std/containers/container.node/node_handle.empty.nodiscard.verify.cpp

17

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; }

[• Quuxplusone]

libcxx/test/std/containers/container.node/node_handle.empty.nodiscard.verify.cpp

17

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.

[curdeius]

libcxx/test/std/containers/container.node/node_handle.empty.nodiscard.verify.cpp

17

Actually, I'm inclined to add this as an extension in another patch. First, I'd like to add missing tests for P0600.

[Mordante]

Since this is mandated I think it makes sense to add the test here. IMO it doesn't belong to the extension tests.

libcxx/test/std/containers/container.node/node_handle.empty.nodiscard.verify.cpp

26

This is indeed a compile-time test. It tests whether the expected compiler diagnostics are issues. Basically this is our typical clang unit test.

[ldionne]

I wasn't aware of this change and I did the same thing in D109027. Apologies for the duplicated effort.

[ldionne]

Abandoning since this was done in a different change, to clear up the review queue and help avoid these issues!

libcxx/docs/Cxx2aStatus.rst

 
 .. csv-table::
    :file: Cxx2aStatusPaperStatus.csv
    :header-rows: 1
    :widths: auto
 
 .. note::
 
-   .. [#note-P0600] P0600: The missing bits in P0600 are in |sect|\ [mem.res.class], |sect|\ [mem.poly.allocator.class], and |sect|\ [container.node.overview].
+   .. [#note-P0600] P0600: The missing bits in P0600 are in |sect|\ [mem.res.class] and |sect|\ [mem.poly.allocator.class].
    .. [#note-P0966] P0966: It was previously erroneously marked as complete in version 8.0. See `bug 45368 <https://llvm.org/PR45368>`__.
 
    .. [#note-P0619] P0619: Only ``std::allocator`` part is implemented.
 
 
 .. _issues-status-cxx2a:
 
 Library Working Group Issues Status
 ====================================
 
 .. csv-table::
    :file: Cxx2aStatusIssuesStatus.csv
    :header-rows: 1
    :widths: auto
 
 Last Updated: 24-Nov-2020

libcxx/include/__node_handle

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___NODE_HANDLE
 #define _LIBCPP___NODE_HANDLE
 
+/*
+
+template<unspecified>
+class node-handle {
+public:
+  using value_type     = see below;     // not present for map containers
+  using key_type       = see below;     // not present for set containers
+  using mapped_type    = see below;     // not present for set containers
+  using allocator_type = see below;
+
+private:
+  using container_node_type = unspecified;                  // exposition only
+  using ator_traits = allocator_traits<allocator_type>;     // exposition only
+
+  typename ator_traits::template
+    rebind_traits<container_node_type>::pointer ptr_;       // exposition only
+  optional<allocator_type> alloc_;                          // exposition only
+
+public:
+  // [container.node.cons], constructors, copy, and assignment
+  constexpr node-handle() noexcept : ptr_(), alloc_() {}
+  node-handle(node-handle&&) noexcept;
+  node-handle& operator=(node-handle&&);
+
+  // [container.node.dtor], destructor
+  ~node-handle();
+
+  // [container.node.observers], observers
+  value_type& value() const;            // not present for map containers
+  key_type& key() const;                // not present for set containers
+  mapped_type& mapped() const;          // not present for set containers
+
+  allocator_type get_allocator() const;
+  explicit operator bool() const noexcept;
+  [[nodiscard]] bool empty() const noexcept; // nodiscard since C++20
+
+  // [container.node.modifiers], modifiers
+  void swap(node-handle&)
+    noexcept(ator_traits::propagate_on_container_swap::value ||
+             ator_traits::is_always_equal::value);
+
+  friend void swap(node-handle& x, node-handle& y) noexcept(noexcept(x.swap(y))) {
+    x.swap(y);
+  }
+};
+
+*/
+
 #include <__config>
 #include <memory>
 #include <optional>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 

libcxx/test/std/containers/container.node/node_handle.empty.nodiscard.verify.cpp

+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// #include <map>
+// #include <set>
+// #include <unordered_map>
+// #include <unordered_set>
+
+// class node-handle;
+//   [[nodiscard]] bool empty() const noexcept; // nodiscard since C++20
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
+#include <map>
+#include <set>
+#include <unordered_map>
+#include <unordered_set>
+
+#include "test_macros.h"
+
+template <typename Container>
+void test_one(Container c) {
+  auto n = c.extract(c.begin());
+  n.empty(); // expected-warning 8{{ignoring return value of function declared with 'nodiscard' attribute}}
+}
+
+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}});
+}