[PATCH 2/3] Factor out duplicated WindowOptPtr initialization.

[Jamey Sharp]

On 11/20/11, walter harms <wharms at bfs.de> wrote:
> Am 20.11.2011 12:48, schrieb Jamey Sharp:
>> diff --git a/dix/window.c b/dix/window.c
>> index 44bfa18..c87020d 100644
>> --- a/dix/window.c
>> +++ b/dix/window.c
>> @@ -152,6 +152,12 @@ static unsigned char _back_msb[4] = {0x11, 0x44,
>> 0x22, 0x88};
>>  static Bool WindowParentHasDeviceCursor(WindowPtr pWin,
>>                                          DeviceIntPtr pDev,
>>                                          CursorPtr pCurs);
>> +static WindowOptPtr
>> +AllocateWindowOptional(CursorPtr cursor,
>> +                       VisualID visual,
>> +                       Colormap colormap,
>> +                       Mask dontPropagateMask);
>> +
>>  static Bool
>>  WindowSeekDeviceCursor(WindowPtr pWin,
>>                         DeviceIntPtr pDev,
>> @@ -481,25 +487,11 @@ CreateRootWindow(ScreenPtr pScreen)
>>      pWin->parent = NullWindow;
>>      SetWindowToDefaults(pWin);
>>
>> -    pWin->optional = malloc(sizeof (WindowOptRec));
>> +    pWin->optional = AllocateWindowOptional(None,
>> +            pScreen->rootVisual, pScreen->defColormap, 0);
>>      if (!pWin->optional)
>>          return FALSE;
>>
>> -    pWin->optional->dontPropagateMask = 0;
>> -    pWin->optional->otherEventMasks = 0;
>> -    pWin->optional->otherClients = NULL;
>> -    pWin->optional->passiveGrabs = NULL;
>> -    pWin->optional->userProps = NULL;
>> -    pWin->optional->backingBitPlanes = ~0L;
>> -    pWin->optional->backingPixel = 0;
>> -    pWin->optional->boundingShape = NULL;
>> -    pWin->optional->clipShape = NULL;
>> -    pWin->optional->inputShape = NULL;
>> -    pWin->optional->inputMasks = NULL;
>> -    pWin->optional->deviceCursors = NULL;
>> -    pWin->optional->colormap = pScreen->defColormap;
>> -    pWin->optional->visual = pScreen->rootVisual;
>> -
>>      pWin->nextSib = NullWindow;
>>
>>      pWin->drawable.id = FakeClientID(0);
>> @@ -3500,18 +3492,21 @@ CheckWindowOptionalNeed (WindowPtr w)
>>   * values.
>>   */
>>
>> -Bool
>> -MakeWindowOptional (WindowPtr pWin)
>> +static WindowOptPtr
>> +AllocateWindowOptional (CursorPtr cursor,
>> +                        VisualID visual,
>> +                        Colormap colormap,
>> +                        Mask dontPropagateMask)
>>  {
>> -    WindowOptPtr optional;
>> -    WindowOptPtr parentOptional;
>> -
>> -    if (pWin->optional)
>> -	return TRUE;
>> -    optional = malloc(sizeof (WindowOptRec));
>> +    WindowOptPtr optional = malloc(sizeof (WindowOptRec));
>>      if (!optional)
>> -	return FALSE;
>> -    optional->dontPropagateMask =
>> DontPropagateMasks[pWin->dontPropagate];
>> +	return NULL;
>> +    optional->cursor = cursor;
>> +    if (cursor)
>> +	cursor->refcnt++;
>> +    optional->visual = visual;
>> +    optional->colormap = colormap;
>> +    optional->dontPropagateMask = dontPropagateMask;
>>      optional->otherEventMasks = 0;
>>      optional->otherClients = NULL;
>>      optional->passiveGrabs = NULL;
>> @@ -3523,21 +3518,22 @@ MakeWindowOptional (WindowPtr pWin)
>>      optional->inputShape = NULL;
>>      optional->inputMasks = NULL;
>>      optional->deviceCursors = NULL;
>> +    return optional;
>> +}
>>
>
> hi,
>
> maybe it is better to use calloc() here to make sure everything is
> initialised ?

That's appealing, but I actually feel better in this case just
checking that all structure fields declared in the header file are
initialized here. It makes you think about what the right
initialization value is, which is important because these fields have
default values specified by the X protocol.

> I have no clue what you are actually changing but i notice that this is
> missing:
> pWin->optional->backingBitPlanes = ~0L;

No, that's in AllocateWindowOptional, in the section that was copied
unchanged from MakeWindowOptional.

> and if you use "ScreenPtr pScreen" instead of "VisualID visual","Colormap
> colormap"
> that would offer you more flexibility in the future.

That only works for one of the callers. The other has to inherit those
values from the nearest ancestor that has an "optional" record.

> btw;
>   -    optional->dontPropagateMask =
> DontPropagateMasks[pWin->dontPropagate];
>   +    optional->dontPropagateMask = dontPropagateMask;
>
> it seems that it was set to 0 after that
>     -    pWin->optional->dontPropagateMask = 0;
>
> but is that intended ?

Again, those are two different callers. Perhaps you'd prefer to read
the full source before and after the patch, rather than the diff?

Jamey