[libcxx-commits] [PATCH] D110852: [libc++] Use addressof in assignment operator.

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

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/test/std/containers/associative/map/map.cons/copy_assign.compile.pass.cpp:22
+#include "test_macros.h"
+#include "deleted_operator_ampersand.h"
+
----------------
The pattern I use for "ADL-proofing tests" is `find libcxx/test/ -name robust_against_adl.pass.cpp`:
```
struct Incomplete;
template<class T> struct Holder { T t; };

std::map<int, Holder<Incomplete>*> m;
m = m;
```
However, the problem here is that `std::map` actually is not fully ADL-proof, and cannot be made fully ADL-proof, because it relies on comparing `allocator<pair<int, Holder<Incomplete>*>>` objects with ADL `operator!=`. So I guess the deleted-specific-function version is the best we can do.


================
Comment at: libcxx/test/std/containers/associative/map/map.cons/copy_assign.compile.pass.cpp:26-28
+    std::map<int, deleted_operator_ampersand> mo;
+    std::map<int, deleted_operator_ampersand> m;
+    m = mo;
----------------
All of these tests can be one line shorter.
```
std::map<int, deleted_operator_ampersand> x;
x = x;
```


================
Comment at: libcxx/test/std/containers/container.adaptors/queue/queue.defn/assign_copy.compile.pass.cpp:9
+
+// UNSUPPORTED: c++03
+
----------------
Throughout: Why is this unsupported in '03?
...oh, because of the `=delete`? Clang supports `=delete` in '03, so we can go ahead and test it. This will shorten most of these test files by another 2 lines each.


================
Comment at: libcxx/test/support/deleted_operator_ampersand.h:18
+///
+/// The class has some additiona operations to be usable in all containers.
+struct deleted_operator_ampersand {
----------------
CaseyCarter wrote:
> typo: "additional"
`additional`


================
Comment at: libcxx/test/support/deleted_operator_ampersand.h:22-23
+  bool operator==(const deleted_operator_ampersand&) const { return true; }
+  template <typename T>
+  friend void operator&(T&&) = delete;
+};
----------------
Let's delete the rest of the evil operators while we're here.
```
template<class T> friend void operator&(T&&) = delete;
template<class T, class U> friend void operator,(T&&, U&&) = delete;
template<class T, class U> friend void operator&&(T&&, U&&) = delete;
template<class T, class U> friend void operator||(T&&, U&&) = delete;
```
You can call the class something like `class OperatorHijacker` or `class EvilOperators` to keep the lines a bit shorter at the call-sites.


================
Comment at: libcxx/test/support/deleted_operator_ampersand.h:26-31
+namespace std {
+template <>
+struct hash<deleted_operator_ampersand> {
+  std::size_t operator()(deleted_operator_ampersand const&) const noexcept { return 0; }
+};
+} // namespace std
----------------
```
template<>
struct std::hash<WhateverName> {
    size_t operator()(const WhateverName&) const { return 0; }
};
```
(Never reopen namespace std from user code. Don't put `noexcept` cruft except where the library needs it.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110852