This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] switch debug.cpp from using std::mutex to __libcpp_mutex_t
Needs ReviewPublic

Authored by DanielMcIntosh-IBM on Jan 14 2022, 5:02 PM.

Details

Reviewers
ldionne
Mordante
Quuxplusone
Group Reviewers
Restricted Project
Summary

This is the 3rd of 4 changes to add support for POSIX(OFF) on z/OS.
See D117366 for more background, and D110349 for discussion of an
alternative implementation.

Diff Detail

Event Timeline

DanielMcIntosh-IBM requested review of this revision.Jan 14 2022, 5:02 PM
DanielMcIntosh-IBM created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2022, 5:02 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
DanielMcIntosh-IBM edited the summary of this revision. (Show Details)Jan 14 2022, 5:17 PM
libcxx/src/debug.cpp
73

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. ;)

263–293

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

Address reveiw comments

libcxx/src/debug.cpp
73

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.

263–293

I'd considered that, but wasn't sure if it was worth it. I've changed it now though.

libcxx/src/debug.cpp
69
73

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.

78
263–293

So worth it. :)