[Mesa-dev] [PATCH v2] mesa/main: Fix memset in formatquery.c

[Alejandro Piñeiro]

On 27/03/17 09:19, Gustaw Smolarczyk wrote:
> 27 mar 2017 08:47 "Alejandro Piñeiro" <apinheiro at igalia.com
> <mailto:apinheiro at igalia.com>> napisał(a):
>
>     On 27/03/17 01:51, Edward O'Callaghan wrote:
>     > V.1:
>     > We memset number of elements without multiplication by the
>     > element size.
>
>     It is not usual to summarize v1. The idea is explain just the changes.
>     >
>     > V.2:
>     > We explicitly set each member to -1 over using a confusing
>     > memset().
>
>     Nitpick: usually, the info about version changes doesn't follow that
>     style (uppercase v, point, number => lowercase v, number).
>
>     >
>     > Signed-off-by: Edward O'Callaghan <funfunctor at folklore1984.net
>     <mailto:funfunctor at folklore1984.net>>
>     > ---
>     >  src/mesa/main/formatquery.c | 2 +-
>     >  1 file changed, 1 insertion(+), 1 deletion(-)
>     >
>     > diff --git a/src/mesa/main/formatquery.c
>     b/src/mesa/main/formatquery.c
>     > index 598d34d..6aa57ec 100644
>     > --- a/src/mesa/main/formatquery.c
>     > +++ b/src/mesa/main/formatquery.c
>     > @@ -1564,7 +1564,7 @@ _mesa_GetInternalformati64v(GLenum target,
>     GLenum internalformat,
>     >      * no pname can return a negative value, we fill params32
>     with negative
>     >      * values as reference values, that can be used to know what
>     copy-back to
>     >      * params */
>     > -   memset(params32, -1, 16);
>     > +   for (i = 0; i < realSize; i++) params32[i] = -1;
>
>     If you keep a loop, then what Brian already said.
>
>     But out of curiosity, as mentioned, the other alternative is filling
>     params32 with -1 when defining it:
>          GLint params32[16] = { [0 ... 15] = -1 };
>
>     Any disadvantage over the loop?
>
>
> If realSize < 16, this form will write more bytes. However if the fill
> size is known at compile time, the compiler should be able to optimize
> the code more, like using 4 sse writes without any loop instead of
> up-to-16 dword ones in a loop.

Ah true, I didn't realize that it was filling just realSize.

Ok, thanks.

So, with the suggested changes (my nitpick on the commit message,
Brian's comment on the loop style):
Reviewed-by: Alejandro Piñeiro <apinheiro at igalia.com>

BR