[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:69
+  bool owns_lock = true;
+  db_guard() { __libcpp_mutex_lock(&__db_mut); }
+  ~db_guard() {
----------------



================
Comment at: libcxx/src/debug.cpp:78
+struct db_guard {
+  void release() {};
+};
----------------



================
Comment at: libcxx/src/debug.cpp:73
+struct db_guard {};
+#endif
 
----------------
DanielMcIntosh-IBM wrote:
> Quuxplusone wrote:
> > 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. ;)
> > 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.
> 
> Having separate structs for read and write locks to me would indicate that there was some need to separate them - e.g. a subtle difference in behaviour somehow, or at the very least the behaviour is likely to diverge in the future in a way that will be hard to implement retroactively if we don't keep them separate now. Neither of these is the case here, and IMO separating them sends the wrong message. It also feels akin to premature optimization - extra work done to solve a non-existent problem.
> 
> hard to implement retroactively if we don't keep them separate now

FWIW, my intuition is basically that exact thing you said; the "hard" part is merely that the future programmer will have to figure out which of the mutex locks are "locking for reading" and which are "locking for writing," and then every code-reviewer on their review will have to check their work. We've already done the work (I assume), so I think we should keep it. (And keeping it as code makes more sense than keeping it as comments, even if the code is tantamount to comments anyway.)
```
using db_read_guard = db_guard;
using db_write_guard = db_guard;
```
Pay 3 lines of code, to save 15 lines of comments. Worth it IMO.


================
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
----------------
DanielMcIntosh-IBM wrote:
> Quuxplusone wrote:
> > 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.)
> I'd considered that, but wasn't sure if it was worth it. I've changed it now though.
//So// worth it. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117373