[Mesa-dev] [PATCH] mesa: assorted clean-ups in detach_shader()

[Ian Romanick]

On 02/10/2014 03:43 PM, Brian Paul wrote:
> Fix formatting, add new comments, get rid of extraneous indentation.
> Suggested by Ian in bug 74723.
> ---
>  src/mesa/main/shaderapi.c |   24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> index 97a57a4..44b4c3a 100644
> --- a/src/mesa/main/shaderapi.c
> +++ b/src/mesa/main/shaderapi.c
> @@ -383,31 +383,31 @@ detach_shader(struct gl_context *ctx, GLuint program, GLuint shader)
>           _mesa_reference_shader(ctx, &shProg->Shaders[i], NULL);
>  
>           /* alloc new, smaller array */

I was looking at this code earlier today.  Would this be better?

         /* Move the "tail" of the list of shaders up by one element.
          */
         memmove(&shProg->Shaders[j], &shProg->Shaders[j + 1],
                 sizeof(shProg->Shaders[0]) * (n - (j + 1)));

         /* Compact the array.  Not worried about compacting realloc failures.
          */
         newList = realloc(shProg->Shaders,
                           sizeof(shProg->Shaders[0]) * (n - 1));
         if (!newList) {
            _mesa_error(ctx, GL_OUT_OF_MEMORY, "glDetachShader");
            return;
         }

         shProg->Shaders = newList;
         shProg->NumShaders = n - 1;

For that matter, do we even need to compact the allocation?

         /* Move the "tail" of the list of shaders up by one element.
          */
         memmove(&shProg->Shaders[j], &shProg->Shaders[j + 1],
                 sizeof(shProg->Shaders[0]) * (n - (j + 1)));

         shProg->Shaders[n - 1] = 0;
         shProg->NumShaders = n - 1;

Whichever way you feel like doing it, they're all improvements.

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

> -         newList =
> -            malloc((n - 1) * sizeof(struct gl_shader *));
> +         newList = malloc((n - 1) * sizeof(struct gl_shader *));
>           if (!newList) {
>              _mesa_error(ctx, GL_OUT_OF_MEMORY, "glDetachShader");
>              return;
>           }
> +         /* Copy old list entries to new list, skipping removed entry at [i] */
>           for (j = 0; j < i; j++) {
>              newList[j] = shProg->Shaders[j];
>           }
> -         while (++i < n)
> +         while (++i < n) {
>              newList[j++] = shProg->Shaders[i];
> -         free(shProg->Shaders);
> +         }
>  
> +         /* Free old list and install new one */
> +         free(shProg->Shaders);
>           shProg->Shaders = newList;
>           shProg->NumShaders = n - 1;
>  
>  #ifdef DEBUG
> -         /* sanity check */
> -         {
> -            for (j = 0; j < shProg->NumShaders; j++) {
> -               assert(shProg->Shaders[j]->Type == GL_VERTEX_SHADER ||
> -                      shProg->Shaders[j]->Type == GL_GEOMETRY_SHADER ||
> -                      shProg->Shaders[j]->Type == GL_FRAGMENT_SHADER);
> -               assert(shProg->Shaders[j]->RefCount > 0);
> -            }
> +         /* sanity check - make sure the new list's entries are sensible */
> +         for (j = 0; j < shProg->NumShaders; j++) {
> +            assert(shProg->Shaders[j]->Type == GL_VERTEX_SHADER ||
> +                   shProg->Shaders[j]->Type == GL_GEOMETRY_SHADER ||
> +                   shProg->Shaders[j]->Type == GL_FRAGMENT_SHADER);
> +            assert(shProg->Shaders[j]->RefCount > 0);
>           }
>  #endif
>