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

Fri Mar 18 10:56:06 PDT 2022

ldionne added a comment.

The important bit is this (emphasis mine):

> A type is considered non-trivial for the purposes of calls if:
>
> - it has a non-trivial copy constructor, move constructor, or destructor, or
> - **all** of its copy and move constructors are deleted.

I went through all the changes in this patch, and I think the key question we need an answer to is:

When a copy assignment operator or copy constructor is explicitly deleted, there is no implicit move constructor generated by the compiler. But does the compiler not declare any move constructor, or does it declare one and mark it as deleted? I wasn't able to find out by reading http://eel.is/c++draft/class.copy.ctor, but it may be easy to answer for others here.

This is relevant because some classes go from having a user-declared private `operator=(T const&)` to having a deleted one. I believe it's possible for such a type to be trivial for the purpose of calls. However, if we change it to `operator(T const&) = delete` **and** the compiler then implicitly declares `T(T const&) = delete`, we'd go from trivial to non-trivial.

Anyway, I've left some not-done comments on the ones where I'm not 100% confident that this patch isn't changing behavior. I suspect it is correct, I just need to understand why.

Note: In my review, TFPC stands for "Trivial For the Purpose of Call"

================
Comment at: libcxx/include/__bit_reference:159

+    __bit_const_reference& operator=(const __bit_const_reference&) = delete;
+
----------------
I'm not sure about this one, see question.

================
Comment at: libcxx/include/__functional/function.h:262
 public:
+    __base(const __base&) = delete;
+    __base& operator=(const __base&) = delete;
----------------
Code was not TFPC before, and it's not after because there's a user-defined destructor.

================
Comment at: libcxx/include/__locale:61

+  __libcpp_locale_guard(__libcpp_locale_guard const&) = delete;
+  __libcpp_locale_guard& operator=(__libcpp_locale_guard const&) = delete;
----------------
Code was not TFPC before, and it's not after because there's a user-defined destructor.

================
Comment at: libcxx/include/__split_buffer:76

+    __split_buffer(const __split_buffer&) = delete;
+    __split_buffer& operator=(const __split_buffer&) = delete;
----------------
Code was not TFPC, and isn't after patch because we have a move ctor but it is user-defined.

================
Comment at: libcxx/include/deque:1042
 public:
+    __deque_base(const __deque_base& __c) = delete;
+    __deque_base& operator=(const __deque_base& __c) = delete;
----------------
Code was not TFPC before, and it's not after because there's a user-defined destructor.

================
Comment at: libcxx/include/forward_list:516

-private:
-    __forward_list_base(const __forward_list_base&);
-    __forward_list_base& operator=(const __forward_list_base&);
+    __forward_list_base(const __forward_list_base&) = delete;
+    __forward_list_base& operator=(const __forward_list_base&) = delete;
----------------
Code was not TFPC before, and it's not after because there's a user-defined destructor.

================
Comment at: libcxx/include/future:1623
 public:
-    _LIBCPP_INLINE_VISIBILITY
-    __packaged_task_base() {}
-    _LIBCPP_INLINE_VISIBILITY
-    virtual ~__packaged_task_base() {}
+    _LIBCPP_HIDE_FROM_ABI __packaged_task_base() {}
+    _LIBCPP_HIDE_FROM_ABI virtual ~__packaged_task_base() {}
----------------
Code was not TFPC before, and it's not after because there's a user-defined destructor.

================
Comment at: libcxx/include/iosfwd:286
+
+    __save_flags(const __save_flags&) = delete;
+    __save_flags& operator=(const __save_flags&) = delete;
----------------
Code was not TFPC before, and it's not after because there's a user-defined destructor.

================
Comment at: libcxx/include/list:527
 public:
+    __list_imp(const __list_imp&) = delete;
+    __list_imp& operator=(const __list_imp&) = delete;
----------------
Can we move these down near the other constructors?

================
Comment at: libcxx/include/list:529
+    __list_imp& operator=(const __list_imp&) = delete;
+
     typedef _Alloc                                                  allocator_type;
----------------
Code was not TFPC before, and it's not after because there's a user-defined destructor.

================
Comment at: libcxx/include/locale:3652

+    wstring_convert(const wstring_convert& __wc) = delete;
+    wstring_convert& operator=(const wstring_convert& __wc) = delete;
----------------
Code was not TFPC before, and it's not after because there's a user-defined destructor.

================
Comment at: libcxx/include/locale:3924

+    wbuffer_convert(const wbuffer_convert&) = delete;
+    wbuffer_convert& operator=(const wbuffer_convert&) = delete;
----------------
Code was not TFPC before, and it's not after because there's a user-defined destructor.

================
Comment at: libcxx/include/map:692

+    __map_node_destructor& operator=(const __map_node_destructor&) = delete;
+
----------------
I'm not sure about this one. I think it was TFPC before the patch, because there were no copy/move ctors declared.

After the patch, there shouldn't be any copy/move ctor declared unless the compiler declares them as deleted, in which case it would become non-TFPC.

================
Comment at: libcxx/include/regex:1433

+    __node(const __node&) = delete;
+    __node& operator=(const __node&) = delete;
----------------
Code was not TFPC before, and it's not after because there's a user-defined destructor.

================
Comment at: libcxx/include/regex:2210

+    __match_char(const __match_char&) = delete;
+    __match_char& operator=(const __match_char&) = delete;
----------------
I'm not sure about this one, see question.

================
Comment at: libcxx/include/regex:2251

+    __match_char_icase(const __match_char_icase&) = delete;
+    __match_char_icase& operator=(const __match_char_icase&) = delete;
----------------
I'm not sure about this one, see question.

================
Comment at: libcxx/include/regex:2293

+    __match_char_collate(const __match_char_collate&) = delete;
+    __match_char_collate& operator=(const __match_char_collate&) = delete;
----------------
I'm not sure about this one, see question.

================
Comment at: libcxx/include/regex:2349

+    __bracket_expression(const __bracket_expression&) = delete;
+    __bracket_expression& operator=(const __bracket_expression&) = delete;
----------------
I'm not sure about this one, see question.

================
Comment at: libcxx/include/regex:3147

+    __lookahead(const __lookahead&) = delete;
+    __lookahead& operator=(const __lookahead&) = delete;
----------------
I'm not sure about this one, see question.

================
Comment at: libcxx/include/thread:153

+    __thread_specific_ptr(const __thread_specific_ptr&) = delete;
+    __thread_specific_ptr& operator=(const __thread_specific_ptr&) = delete;
----------------
Code was not TFPC before, and it's not after because there's a user-defined destructor.

================
Comment at: libcxx/include/tuple:301
     __tuple_leaf(__tuple_leaf&& __t) = default;
+    __tuple_leaf& operator=(const __tuple_leaf&) = delete;

----------------
This one has defaulted copy and move constructors, so this won't change triviality.

================
Comment at: libcxx/include/tuple:363

     __tuple_leaf(__tuple_leaf const &) = default;
     __tuple_leaf(__tuple_leaf &&) = default;
----------------
Quuxplusone wrote:
> 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.
This one has defaulted copy and move constructors, so this won't change triviality.

================
Comment at: libcxx/include/unordered_map:780

+    __hash_map_node_destructor& operator=(const __hash_map_node_destructor&) = delete;
+
----------------
This one is not TFPC before and not TFPC after because it has user-defined copy/move ctors.

================
Comment at: libcxx/include/vector:787

+    _ConstructTransaction(_ConstructTransaction const&) = delete;
+    _ConstructTransaction& operator=(_ConstructTransaction const&) = delete;
----------------
Code was not TFPC before, and it's not after because there's a user-defined destructor.

Also this type should not be ABI affecting, but w/e.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121213