[libcxx-commits] [PATCH] D115291: [libc++] `= delete` member functions with // = delete;

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

Quuxplusone accepted this revision.
Quuxplusone added inline comments.


================
Comment at: libcxx/include/__locale:211
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR id() :__id_(0) {}
+    void operator=(const id&) = delete;
+    id(const id&) = delete;
----------------
jloser wrote:
> Interesting to see a copy assignment operator with //return type of void//. Is that actually intended? I suspect this meant to be `id& operator=(const id&) = delete;` surely, right?
> 
> Ditto with `seed_seq` below in another file.
The return type of a deleted overload is unobservable, so it doesn't really matter. But this is exactly implementing https://eel.is/c++draft/locale.id — `<locale>` is full of weird return types — so IMHO this diff is good as-is.


================
Comment at: libcxx/include/__mutex_base:146
 
 public:
     _LIBCPP_INLINE_VISIBILITY
----------------
Could remove line 146 now.


================
Comment at: libcxx/include/__random/random_device.h:53
+    random_device(const random_device&) = delete;
+    random_device& operator=(const random_device&) = delete;
 
----------------
http://eel.is/c++draft/rand.device specifies this return type as `void` also. Change it?
Also, I'd actually leave these down around line 60, just to match the order they're specified in [rand.device].


================
Comment at: libcxx/include/__random/seed_seq.h:66-67
 
-private:
-    // no copy functions
-    seed_seq(const seed_seq&); // = delete;
-    void operator=(const seed_seq&); // = delete;
+    seed_seq(const seed_seq&) = delete;
+    void operator=(const seed_seq&) = delete;
 
----------------
This unobservable `void` matches the specification in https://eel.is/c++draft/rand.util.seedseq and therefore LGTM.


================
Comment at: libcxx/include/ios:314
 
 public:
     static bool sync_with_stdio(bool __sync = true);
----------------
Remove line 314?


================
Comment at: libcxx/include/istream:294-295
 
-    sentry(const sentry&); // = delete;
-    sentry& operator=(const sentry&); // = delete;
+    sentry(const sentry&) = delete;
+    sentry& operator=(const sentry&) = delete;
 
----------------
Nit: Make these public (and move them down to line 302) to match https://eel.is/c++draft/istream.sentry


================
Comment at: libcxx/include/mutex:221
+    recursive_mutex(const recursive_mutex&) = delete;
+    recursive_mutex& operator=(const recursive_mutex&) = delete;
 
----------------
Nit: Let's keep a blank line after 219, for consistency with the spec and with line 241.
Ditto after line 283.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115291