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

[Daniel McIntosh via Phabricator via libcxx-commits]

DanielMcIntosh-IBM added inline comments.


================
Comment at: libcxx/src/debug.cpp:73
+struct db_guard {};
+#endif
 
----------------
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.



================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117373