[PATCH] Request review for KSharedDataCache (Round 2)

[Oswald Buddenhagen]

On Wed, Apr 28, 2010 at 09:04:39PM -0400, Michael Pyne wrote:
> On Wednesday, April 28, 2010 06:20:37 Oswald Buddenhagen wrote:
> > On Tue, Apr 27, 2010 at 10:57:48PM -0400, Michael Pyne wrote:
> > > On Sunday, April 25, 2010 15:37:43 Oswald Buddenhagen wrote:
> > > > > +    mutable pthread_mutex_t lockMutex;
> > > > > +    QAtomicInt ready; ///< DO NOT INITIALIZE
> > > > > +    uint       cacheSize;
> > > > > +    uint       cacheAvail;
> > > > > 
> > > > > +    quint8     version;
> > > > 
> > > > it would be wise to put that field first and define that any version
> > > > upgrades may not change that.
> > > > ready should be the second field. no, actually, it should be the first
> > > > and fixed as well, as it is used even prior to checking the version.
> > > 
> > > Well ready is only ever used once in the setup, and likewise with
> > > version, whereas lockMutex, cacheSize, and cacheAvail are used much more
> > > frequently.
> > 
> > yes, but that's besides the point. for the versioning to actually work,
> > the version field itself needs to be version-independent. the easiest
> > way to achieve this is by putting it first.
> 
> Well it's certainly no difference to the implementation whether it reads 20 
> bytes and checks the last 1 to determine the version or whether it reads the 
> first byte as the version.
> 
that's not the point. the idea behind versioning is that you want the
layout to be changeable. but you can't move around the version field -
it must always be in the same place. that means that whatever changes
you make you have to take care that you don't change the offset (and
size, but that's simple) of the version field. by far the simplest way
to achieve that is putting it at the start of the struct.
and obviously the same applies to ready, as it is accessed before the
version field is even checked.