[Mesa-dev] [PATCH 15/70] st/mesa/glsl/nir/i965: make use of new gl_shader_program_data in gl_shader_program

[Emil Velikov]

Hi Tim,

In general we could use the odd local variable to make things shorter
(and cut down the number of derefs).
That can (should?) be done once we're finished with the big churn.

On 11 November 2016 at 00:45, Timothy Arceri
<timothy.arceri at collabora.com> wrote:

> @@ -296,6 +296,8 @@ init_shader_program(struct gl_shader_program *prog)
>     prog->Type = GL_SHADER_PROGRAM_MESA;
>     prog->RefCount = 1;
>
> +   prog->data = create_shader_program_data();
> +
This can fail. Please move it a level up (such that it's symmetric to
the dtor) and call ralloc_free(shProg) on failure.

>     prog->AttributeBindings = string_to_uint_map_ctor();
>     prog->FragDataBindings = string_to_uint_map_ctor();
>     prog->FragDataIndexBindings = string_to_uint_map_ctor();
> @@ -309,7 +311,7 @@ init_shader_program(struct gl_shader_program *prog)
>
>     exec_list_make_empty(&prog->EmptyUniformLocations);
>
> -   prog->InfoLog = ralloc_strdup(prog, "");
> +   prog->data->InfoLog = ralloc_strdup(prog->data, "");
IMHO it's fine keeping this piece here (despite the above suggestion).


> +         _mesa_uniform_detach_all_driver_storage(&shProg->data->
> +                                                    UniformStorage[i]);
Hmm had no idea this is legal. Wondering how many compilers will be
happy with it - worth keeping on single line ?

Please check if we leak due to the missing ctx in
create_shader_program_data() and if we're fine _do_ ignore my
suggestion.

With the small nitpicks patches 12-15 incl. are
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

-Emil