Remove CRITICAL_BLOCK and TRY_CRITICAL_BLOCK, and replace them with LOCK, TRY_LOCK and IS_LOCKED. These are macros that define a local CCriticalBlock variable (a boost::interprocess::scoped_lock wrapper), which is much more C++ than the if and/or for constructions in macros.
I've intentionally not touched formattng/indentation where possible, in order not to break other patches. There are several cases where simplications are possible now, though.
550 | - TRY_CRITICAL_BLOCK(pnode->cs_vRecv) 551 | - TRY_CRITICAL_BLOCK(pnode->cs_mapRequests) 552 | - TRY_CRITICAL_BLOCK(pnode->cs_inventory) 553 | - fDelete = true; 554 | + { 555 | + TRY_LOCK(pnode->cs_vSend);
This code becomes reasonably ugly. Would it be possible to redefine TRY_CRITICAL_BLOCK to use the scoped locks and keep this code (approximately) the same?
You could just have several scoped locks (like with your suggestion below to include a name for the lock), and test whether they all succeed. That would however be different from the code above (which unlocks everything as soon as a failure occurs).
EDIT: The problem to do it correctly with only one line of code is that you do need a conditional block of code somewhere that depends on the lock, which is simply not doable with a scoped lock (except using exceptions maybe...).
289 | @@ -289,6 +290,13 @@ class CMutexLock 290 | #define WAITABLE_CRITICAL_BLOCK(cs) \ 291 | for (bool fcriticalblockonce=true; fcriticalblockonce; assert(("break caught by WAITABLE_CRITICAL_BLOCK!" && !fcriticalblockonce)), fcriticalblockonce=false) \ 292 | for (CWaitableCriticalBlock waitablecriticalblock(cs, #cs, __FILE__, __LINE__); fcriticalblockonce; fcriticalblockonce=false) 293 | +*/ 294 | + 295 | +#define LOCK(cs) CCriticalBlock criticalblock(cs, #cs, __FILE__, __LINE__)
Hm I'd prefer passing the name of the lock ("criticalblock") as a macro parameter to LOCK, TRY_LOCK and IS_LOCKED. Especially the bare IS_LOCKED (with hardcoded name) look too much like magic to my taste, as there is no visual clue what lock it refers to.
Yes, it's a balance between code size and clean code. Note that the current master already has hardcoded names as well...
Well yes but we're trying to improve. IMO it'd be best to completely get rid of the macros, if possible. But making them less "magical" would already be good.
Maybe add the name argument only for TRY_LOCK? For LOCK and LOCK2 you never need to refer to them anyway.
I considered removing all macros and go to pure boost::interprocess, but there is no way to keep Gavin's fancy deadlock detection then, which needs FILE and LINE to track code locations. I'm fine with just an argument for TRY_LOCK and IS_LOCKED.
You mean boost::thread, please no more boost::interprocess :)
OK, let's do that then.
Updated; I like this even more: TRY_LOCK(cs, name); if (name) { ... }
Yes this is much better :)
To understand that stuff, LOCK() is used, when code needs to be thread safe and UNLOCK() is used if that part has been processed?
LOCK() implements what is called a scoped lock: it makes sure that inside the current block of code (surrounded by { }), a lock on a particular mutex is taken, and released when that block of code exits. Because of that, there is no need for an UNLOCK().
24 | } while(0);
25 |
26 | do {
27 | - TRY_CRITICAL_BLOCK(cs)
28 | + TRY_LOCK(cs);
29 | + if (IS_LOCKED)
Where is IS_LOCKED defined?
Oh, right. Leftover from an older version. Fixed now.
ACK.