[libcxx-commits] [PATCH] D117373: [libcxx] switch debug.cpp from using std::mutex to __libcpp_mutex_t

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

Quuxplusone added inline comments.


================
Comment at: libcxx/src/debug.cpp:73
+struct db_guard {};
+#endif
 
----------------
This is great RAII cleanup!
Personally I'm not convinced that it makes sense to lose the tutorial distinction between `WLock` and `RLock`, though. Could you keep it as two structs `db_read_lock` and `db_write_lock`? Or is there a reason you think that that's a bad idea?
One benefit of `db_read_lock guard;` is that you can then eliminate the comment `// Read lock` because it'll be obvious.

Throughout, I think we should stay away from the identifier `_` because people (GNU gettext, C++ pattern-matching proposals, ...) like to mess with it. In other codebases, my convention is `std::lock_guard lk(m);` i.e. using `lk` as the name of the lock variable. Of course `guard` also seems like a good name. ;)


================
Comment at: libcxx/src/debug.cpp:263-293
 {
 #ifndef _LIBCPP_HAS_NO_THREADS
-    mut().lock();
+    __libcpp_mutex_lock(&__db_mut);
 #endif
     if (__cend_ == __cbeg_)
     {
 #ifndef _LIBCPP_HAS_NO_THREADS
----------------
This smells like you need `db_guard` to be `.release()`-able, so that you can eliminate all of the `#if`-stuff and manual unlocking currently here, and just do
```
guard.release();  // Keep the write lock locked when we return non-null
return p;
```
on line 294. (Write lock? Read lock? I didn't look closely enough.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117373