Re: SolarMutexTryAndBuyGuard

On 27/09/14 15:10, Miklos Vajna wrote:
> Hi Michael,
>
> On Fri, Sep 26, 2014 at 02:33:38PM -0700, Michael Stahl <mstahl@redhat.com> wrote:
> > +class SolarMutexTryAndBuyGuard
> > +    : private boost::noncopyable
> > +{
> > +    private:
> > +        bool m_isAcquired;
> > +#if OSL_DEBUG_LEVEL > 0
> > +        bool m_isChecked;
> > +#endif
>
> Isn't this exactly the situation when we should use DBG_UTIL and not
> OSL_DEBUG_LEVEL -- i.e. when the conditional part results in a different
> class layout?

in general, you are of course right.

but this all-inline guard class can only sensibly be used within a
single function, with the life time limited to a single stack frame;
this should imply that the situation where multiple compilation units
built with different OSL_DEBUG_LEVEL access the same instance with
different layout cannot happen.

on the other hand, you have a point in that inexperienced developers
could read this class and assume that all additional class members
should be guarded in this way; perhaps it would be better to change it
to DBG_UTIL just for consistency.

well, generally speaking all uses of SolarMutex::tryToAcquire() are
broken by design anyway...

on a related note, does anybody know what commit
6ea3c24201b2d4255306f6e745e242567f3bbb8c is trying to do with
SolarMutex?  it clearly looks to me that the error handling paths in
vcl/unx/source/dtrans/X11_selection.cxx will not release the SolarMutex
again in case it succeeds in acquiring it, which sounds like a bad idea.