doc: Document locks - increase LOCK(...) comment coverage from 2 % to 77 %

[practicalswift]

Currently only a tiny fraction of all LOCK(...):s are documented with information on why each lock is held.

In the absence of such comments it can sometimes be hard to see why a specific lock is held from merely reading the code. This risks leading to situations where redundant LOCK(...):s are kept by mistake or "just to be safe" after refactoring, etc.

This PR improves the situation by adding comments with information on why a specific lock is being held by adding a comment using the form:

LOCK(cs_lockName); // variableBeingGuardedByThisLock, FunctionRequiringHoldingThisLock(...)
…
variableBeingGuardedByThisLock += 1;
…
FunctionRequiringHoldingThisLock(someThing);

Before this patch there were 9 documented and 514 undocumented LOCK/LOCK2:s –

$ git grep -E "[^a-zA-Z_](LOCK|LOCK2)\(" -- "*.h" "*.cpp" | grep -v sync.h | grep "//" | wc -l
9
$ git grep -E "[^a-zA-Z_](LOCK|LOCK2)\(" -- "*.h" "*.cpp" | grep -v sync.h | grep -v "//" | wc -l
514

After this patch there are 405 documented and 118 undocumented LOCK/LOCK2:s –

$ git grep -E "[^a-zA-Z_](LOCK|LOCK2)\(" -- "*.h" "*.cpp" | grep -v sync.h | grep "//" | wc -l
405
$ git grep -E "[^a-zA-Z_](LOCK|LOCK2)\(" -- "*.h" "*.cpp" | grep -v sync.h | grep -v "//" | wc -l
118

Note that there are still some locks left that are undocumented. I need help identifying the variable that is ultimately being guarded by each of these locks (it could be that some of these locks are unnecessary - if so, let me know!).

Please help by browsing the remaining undocumented locks using ...

$ git grep -3 -n -E '^[^/#]*[^A-Z_](LOCK|LOCK2)\([^/]*$' -- "*.cpp" "*.h"

... and see if you can help bring clarity. Each undocumented guarded variable that can be identified helps a lot - just leave a comment on the missing lock/guarded variable pair you've found and I'll track down all instances of that combination and add the appropriate comments :-)

That will be of great help for this comment PR and also for the related thread-safety annotations PR that I'm working on (see #11226).

[meshcollider]

Big concept ACK 👍

[jnewbery]

concept ACK. I think this is especially helpful for new contributors to the project.

[theuni]

mmm, I like the idea, but I don't agree with annotating each lock with a comment. They will eventually go stale and be misleading or completely wrong.

Instead, I'd rather annotate everything with GUARDED_BY (ala #11226), so that it's easy to see what things a mutex protects, as well as having it enforceable via static analysis.

[practicalswift]

@theuni You have a point regarding the risk for staleness unless we are careful to keep them up-to-date. If there is any interest I can commit to keeping these comments up-to-date - including removing no longer needed locks (due to refactoring, etc.).

FWIW, I have written a script that generates these comments automatically by using the GUARDED_BY annotations added in #11226. The comments added in this PR were created by running that script.

[practicalswift]

Rebased! :-)

[jonasschnelli]

Seems NACKish to me. Will quickly run out of date and – if you ask me – you should follow/read the code to understand the lock rather then trusting a comment.

[maflcko]

Agree with Cory. We might not need those annotations after #11226 is merged, as that pull already serves as documentation.

[fanquake]

Needs a rebase, but going to close this in favour of #11226. Seems like this sort of change could end up with a similar situation to what happened after enabling -Wshadow. Also seems like the EXCLUSIVE_LOCKS_REQUIRED and GUARDED_BY will work better as documentation themselves. @laanwj Any thoughts?

[laanwj]

@fanquake I agree, annotations that can be checked/enforced by the compiler are preferable. This is a nice initiative in principle but keeping it up to date is unrealistic.

…

On Sep 25, 2017 03:41, "fanquake" ***@***.***> wrote: Needs a rebase, but going to close this in favour of #11226 <#11226>. Seems like this sort of change could end up with a similar situation to what happened after enabling -Wshadow. Also seems like the EXCLUSIVE_LOCKS_REQUIRED and GUARDED_BY will work better as documentation themselves. @laanwj <https://github.com/laanwj> Any thoughts? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11369 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHutp9YP7AZlLBpBiM5h4TWuX26XOLSks5slwTIgaJpZM4PcWq4> .