On Mon, 2008-10-06 at 16:07 -0400, Andrew Stitcher wrote: > 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
Agreed that is a problem. In the past I've also used naming conventions like fooUnlocked(). I still don't have any solution that I really like. > - 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. Confused me with that last sentence - the first part says we should use assert(), but the second part says we can't?
