On Mon, 2008-10-06 at 14:23 -0400, Alan Conway wrote: > On Mon, 2008-10-06 at 13:39 -0400, Andrew Stitcher wrote: > > On Mon, 2008-10-06 at 13:29 -0400, Alan Conway wrote: > > > On Mon, 2008-10-06 at 11:38 -0400, Andrew Stitcher wrote: > > > > On Fri, 2008-10-03 at 17:31 -0400, Alan Conway wrote: > > > > > ... > > > > > (1) my usual style being: > > > > > - all public member functions are thread safe. > > > > > - use ScopedLock to acquire/release locks. > > > > > - private, unlocked functions that are only intended to be called > > > > > with > > > > > the lock already held take a dummy extra paramater of type ScopedLock& > > > > > > > > > > The dummy ScopedLock& parameter marks functions that are not locked > > > > > and > > > > > also makes it hard to accidentally call them in an unlocked context. > > > > > > > > This would explain why I occasionally see odd functions with an unused > > > > ScopedLock& parameter, and try and figure out why the person who wrote > > > > it did that. Then sigh, and go and remove the useless parameter before > > > > checking in my changes. > > > > > > > > Did I miss a wiki note/or an email? Oh, such is life. > > > > > > I generally stick a comment in the .h file but very possible I've > > > forgotten on occasion. I sent round an email when I first started doing > > > this, but that's easy to miss. I'll be more careful to include comments > > > in future. > > > > > > Any other useful conventions out there for marking thread safe/unsafe > > > functions? I think the public == thread safe is generally a good rule > > > but I've yet to find a satisfactory way of marking private thread-unsafe > > > functions as such. > > > > Generally I just put a comment (in header file or both) saying that a > > prerequisite for this function is to be holding a particular lock. > > > > I think "the correct" way to do this is to use something like > > "assert(<lock held>);". I think this is the correct way as I think you > > should document (as many as possible) preconditions with asserts. > > However we don't have a way of expressing <lock held> efficiently as far > > as I know. > > The point of the signature change (or Alexandrescus use of volatile) is > to catch errors at compile time rather than run time. Asserts and > comments don't help with that. The idea of the dummy ScopedLock& > parameter is to make it impossible to call the function unless you have > a local ScopedLock or were passed a ScopedLock&, so it's more difficult > to accidentally call an unlocked function when you don't hold the lock.
I understand this. To me the assert approach is easy to follow as it is self documenting (you are explicitly asserting that a specific lock is held), even if it isn't at compile time. The conventions approach - yours and the volatile approach - require some knowledge external to the code to understand. I thought the "volatile" approach had some currency and use, and so would be understood - you've demonstrated that isn't true. Your approach unfortunately isn't self documenting and makes the code look odd to my eyes - I'm willing to go along with it, I just happen to think that using assert() is better in this case - especially as we have no low cost way to do the assertion in any case. A
