[Mesa-dev] [PATCH 19/23] glsl: Add image built-in function generator.

[Francisco Jerez]

Paul Berry <stereotype441 at gmail.com> writes:

>[...]
> I see a few downsides to this approach:
>
> - There is unnecessary code duplication between
> builtin_builder::create_intrinsics() and
> builtin_builder::create_builtins().  Both of them create the same set of
> functions using the same parameters, with the only difference being the
> function names and whether emit_stub() is called.  Furthermore, if the
> names passed to emit_stub() in create_builtins() have to match the function
> names used in create_intrinsics().
>

I've fixed this one issue (and the other minor problems you pointed out)
here [1].

> - builtin_builder::create_intrinsics() and
> builtin_builder::create_builtins() create the same set of
> image_builtin_builder objects every time they are called.  I think the code
> would be easier to follow if the set of image_builtin_builter objects were
> declared as a static const table.
>
> - Currently we don't have any precedent for the use of the fluent interface
> style in Mesa.  While I'm not opposed to it on principle, I don't think
> this particular use of it is very compelling, and I'd prefer to delay
> introducing it until we have a case where the benefit is clearer.
>
>
> Here's what I would recommend doing instead:
>
> - Change image_builtin_builder to a plain struct, and remove the fluent
> setter functions for setting its fields.
>
> - Remove the emit_stub boolean and instead make it an argument to
> add_image_function().
>
> - Remove the name parameter to add_image_function(), and instead store both
> the intrinsic name and the builtin name as separate fields in the struct.
> add_image_function() will choose which name to use based on its emit_stub
> argument.
>
> - At toplevel in buildin_functions.cpp, declare a static const array of
> image_builtin_builder objects, one for each image function.
>
> - In builtin_builder::create_intrinsics(), loop through the static array of
> image_builtin_builders, calling add_image_function() on each and passing
> false for emit_stub.
>
> - In builtin_builder::create_builtins(), loop through the static array of
> image_builtin_builders, calling add_image_function() on each and passing
> true for emit_stub.

Sorry but I don't see the point, the code looks considerably easier to
follow passing parameters this way rather than using anonymous
parameters because of the many arguments which are used or not in
different combinations depending on the intrinsic.  Using "fluent"
parameter passing was an after-the-fact change motivated by the
difficulty of verifying the correctness of the built-in definitions,
which are obviously correct structured this way.

I've actually had what you describe in front of me already and this is
cleaner [though that might well be a matter of personal taste].  I'm not
planning on going back to anonymous arguments and static arrays.

Thanks.

[1] http://cgit.freedesktop.org/~currojerez/mesa/commit/?h=image-load-store&id=02d4e6ee0f7665f87564ee372317fc530e0e0ef6
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131212/9fe97d54/attachment-0001.pgp>