Added comments to sync.h to make it easier to understand the macros #2174

pull CodeShark wants to merge 1 commits into bitcoin:master from CodeShark:sync_macro_clarification changing 1 files +42 −0

CodeShark commented at 9:36 AM on January 13, 2013: contributor

Having taken me a while (i.e. more than the couple seconds it should take) to painstakingly decipher all the synchronization macros, I decided to add some comments to sync.h explaining in simple terms what the macros really translate to once you remove all the excess nonfunctional debug code.

I welcome all of you who have more experience with the code to chime in and point out where I'm going wrong, what to add, and how it can be improved.

I fully support using debugging frameworks - but let's at least clean this up so that it's both easy for a human to read AND easy to set up debugging options - for people who haven't necessarily been dealing with this stuff daily for months.

in src/sync.h:None in 3d07cf2b6b outdated

  24 | + 
  25 | + 
  26 | +CCriticalSection mutex;
  27 | +   boost::recursive_mutex mutex;
  28 | +
  29 | +LOCK(mutex); // uses the local variable criticalblock for RAII

sipa commented at 5:01 PM on January 13, 2013:

Why not write it using the variable name criticalblock below, then?

in src/sync.h:None in 3d07cf2b6b outdated
```
  55 | +
  56 | +
  57 | +
  58 | +///////////////////////////////
  59 | +//                           //
  60 | +// THE MASOCHISTIC DEFINITON //
```
sipa commented at 5:01 PM on January 13, 2013:

Can you use a slightly more respectful word? :p

CodeShark commented at 12:25 AM on January 14, 2013:

I didn't mean it offensively - The stuff is actually pretty clever...it was just having to parse through it all manually only to realize that it boils down to a couple lines of code...I didn't intend for that word to actually remain in the codebase.
BitcoinPullTester commented at 10:51 PM on January 13, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/3d07cf2b6bee9ef1ecfbb54cd8f08904857a22e4 for binaries and test log.
BitcoinPullTester commented at 1:08 AM on January 14, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/fcbc2d8427bfef6066ae3fd5638976548bdd2c42 for binaries and test log.

in src/sync.h:None in fcbc2d8427 outdated

  26 | +CCriticalSection mutex;
  27 | +   boost::recursive_mutex mutex;
  28 | +
  29 | +LOCK(mutex);
  30 | +   boost::unique_lock<boost::recursive_mutex> criticalblock(mutex);
  31 | +   criticalblock.lock();

sipa commented at 8:15 PM on January 14, 2013:

Actually, this lock call is implicit when using the constructor used above.

in src/sync.h:None in fcbc2d8427 outdated
```
  52 | +
  53 | +
  54 | +
  55 | +////////////////////////////////////////////
  56 | +//                                        //
  57 | +// THE ACTUAL CLEVER BUT OPAQUE DEFINITON //
```
sipa commented at 8:16 PM on January 14, 2013:

I think it's weird for code to call itself clever. Sorry for being pedantic about just a comment, but I don't think such statements belong in our source code. How about "The simplified definition" and "The implementation" ?
CodeShark commented at 7:37 PM on January 16, 2013: contributor

I was being facetious in my last commit, obviously...
BitcoinPullTester commented at 8:17 PM on January 16, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f260a599a075f84f220343b2dae4264d8225324c for binaries and test log.
BitcoinPullTester commented at 3:41 AM on January 24, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f260a599a075f84f220343b2dae4264d8225324c for binaries and test log.
sipa commented at 9:44 AM on February 5, 2013: member

ACK if you squeeze the two commits together.
sipa commented at 6:24 PM on April 7, 2013: member

Can you please squeeze these together?
jgarzik commented at 4:52 PM on May 30, 2013: contributor

Poke @CodeShark
sipa commented at 5:20 PM on June 22, 2013: member

Please squeeze? puppyeyes
sipa commented at 12:07 AM on June 25, 2013: member

ACK
Added comments to sync.h to make it easier to understand the macros 042da8bc0d
sipa referenced this in commit 1f2d739ac1 on Jun 26, 2013
sipa merged this on Jun 26, 2013
sipa closed this on Jun 26, 2013
owlhooter referenced this in commit 855ac356ad on Oct 11, 2018
guruvan referenced this in commit 2ccfffc87d on Nov 8, 2018
DrahtBot locked this on Aug 16, 2022

Contributors