[libcxx-commits] [PATCH] D121213: [libc++] Enable modernize-use-equals-delete

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__functional/function.h:264
+    __base& operator=(const __base&) = delete;
+
     _LIBCPP_INLINE_VISIBILITY __base() {}
----------------
FWIW, my personal style on the OOP polymorphic types like `__base` (where they are clearly just abstract base classes with lots of pure virtual methods) is not to bother with the special member functions at all. Nobody's seriously going to be value-semantic-copying this type, so why waste the two LoC preventing it?
But I see the "guardrail against //accidental// copying" argument, so I'm not asking to change this (unless my comment happens to start a new consensus :)).


================
Comment at: libcxx/include/__functional/reference_wrapper.h:37-38
 
     static void __fun(_Tp&) _NOEXCEPT;
-    static void __fun(_Tp&&) = delete;
+    static void __fun(_Tp&&) = delete; // NOLINT(modernize-use-equals-delete) ; This is llvm.org/PR54276
 
----------------
Eliminate the NOLINT comment and just make these two declarations public; it can't hurt.
(I agree that 54276 is a real bug, though.)

Ditto in `ref_view` below.


================
Comment at: libcxx/include/iosfwd:287
+    __save_flags(const __save_flags&) = delete;
+    __save_flags& operator=(const __save_flags&) = delete;
 };
----------------
Stylistically, I'd prefer to see these placed between new lines 278 and 279, so it goes "ctors, assignment operators, dtors." Ditto throughout (at least in the cases where you're already shuffling code around).


================
Comment at: libcxx/include/tuple:363-365
     __tuple_leaf(__tuple_leaf const &) = default;
     __tuple_leaf(__tuple_leaf &&) = default;
+    __tuple_leaf& operator=(const __tuple_leaf&) = delete;
----------------
Consider west-consting line 363 at the same time, and also doing something explicitly about `__tuple_leaf& operator=(__tuple_leaf&&)`. I actually don't know off the top of my head what happens to it right now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121213