[Mesa-dev] [PATCH 2/5] glsl: Get rid of hardcoded arrays of shader target names.
Paul Berry
stereotype441 at gmail.com
Tue Dec 17 11:28:13 PST 2013
On 17 December 2013 10:51, Brian Paul <brianp at vmware.com> wrote:
> On 12/17/2013 11:23 AM, Paul Berry wrote:
>
>> We already have a function for converting a shader type index to a
>> string: _mesa_glsl_shader_target_name().
>> ---
>> src/glsl/link_atomics.cpp | 8 +++-----
>> src/glsl/linker.cpp | 16 ++++++----------
>> 2 files changed, 9 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/glsl/link_atomics.cpp b/src/glsl/link_atomics.cpp
>> index 33903ad..cac5615 100644
>> --- a/src/glsl/link_atomics.cpp
>> +++ b/src/glsl/link_atomics.cpp
>> @@ -21,6 +21,7 @@
>> * DEALINGS IN THE SOFTWARE.
>> */
>>
>> +#include "glsl_parser_extras.h"
>> #include "ir.h"
>> #include "ir_uniform.h"
>> #include "linker.h"
>> @@ -214,9 +215,6 @@ link_check_atomic_counter_resources(struct
>> gl_context *ctx,
>> struct gl_shader_program *prog)
>> {
>> STATIC_ASSERT(MESA_SHADER_TYPES == 3);
>> - static const char *shader_names[MESA_SHADER_TYPES] = {
>> - "vertex", "geometry", "fragment"
>> - };
>> const unsigned max_atomic_counters[MESA_SHADER_TYPES] = {
>> ctx->Const.VertexProgram.MaxAtomicCounters,
>> ctx->Const.GeometryProgram.MaxAtomicCounters,
>> @@ -260,11 +258,11 @@ link_check_atomic_counter_resources(struct
>> gl_context *ctx,
>> for (unsigned i = 0; i < MESA_SHADER_TYPES; i++) {
>> if (atomic_counters[i] > max_atomic_counters[i])
>> linker_error(prog, "Too many %s shader atomic counters",
>> - shader_names[i]);
>> + _mesa_glsl_shader_target_name((gl_shader_type)
>> i));
>>
>> if (atomic_buffers[i] > max_atomic_buffers[i])
>> linker_error(prog, "Too many %s shader atomic counter buffers",
>> - shader_names[i]);
>> + _mesa_glsl_shader_target_name((gl_shader_type)
>> i));
>> }
>>
>> if (total_atomic_counters > ctx->Const.MaxCombinedAtomicCounters)
>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>> index a6133ea..1406f13 100644
>> --- a/src/glsl/linker.cpp
>> +++ b/src/glsl/linker.cpp
>> @@ -1893,10 +1893,6 @@ store_fragdepth_layout(struct gl_shader_program
>> *prog)
>> static void
>> check_resources(struct gl_context *ctx, struct gl_shader_program *prog)
>> {
>> - static const char *const shader_names[MESA_SHADER_TYPES] = {
>> - "vertex", "geometry", "fragment"
>> - };
>> -
>> const unsigned max_samplers[MESA_SHADER_TYPES] = {
>> ctx->Const.VertexProgram.MaxTextureImageUnits,
>> ctx->Const.GeometryProgram.MaxTextureImageUnits,
>> @@ -1929,7 +1925,7 @@ check_resources(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>>
>> if (sh->num_samplers > max_samplers[i]) {
>> linker_error(prog, "Too many %s shader texture samplers",
>> - shader_names[i]);
>> + _mesa_glsl_shader_target_name((gl_shader_type) i));
>> }
>>
>> if (sh->num_uniform_components > max_default_uniform_components[i])
>> {
>> @@ -1938,11 +1934,11 @@ check_resources(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>> "components, but the driver will try to
>> optimize "
>> "them out; this is non-portable out-of-spec "
>> "behavior\n",
>> - shader_names[i]);
>> + _mesa_glsl_shader_target_name((gl_shader_type)
>> i));
>> } else {
>> linker_error(prog, "Too many %s shader default uniform
>> block "
>> "components",
>> - shader_names[i]);
>> + _mesa_glsl_shader_target_name((gl_shader_type)
>> i));
>> }
>> }
>>
>> @@ -1952,10 +1948,10 @@ check_resources(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>> linker_warning(prog, "Too many %s shader uniform
>> components, "
>> "but the driver will try to optimize them
>> out; "
>> "this is non-portable out-of-spec
>> behavior\n",
>> - shader_names[i]);
>> + _mesa_glsl_shader_target_name((gl_shader_type)
>> i));
>> } else {
>> linker_error(prog, "Too many %s shader uniform components",
>> - shader_names[i]);
>> + _mesa_glsl_shader_target_name((gl_shader_type)
>> i));
>> }
>> }
>> }
>> @@ -1979,7 +1975,7 @@ check_resources(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>> for (unsigned i = 0; i < MESA_SHADER_TYPES; i++) {
>> if (blocks[i] > max_uniform_blocks[i]) {
>> linker_error(prog, "Too many %s uniform blocks (%d/%d)",
>> - shader_names[i],
>> + _mesa_glsl_shader_target_name((gl_shader_type)
>> i),
>> blocks[i],
>> max_uniform_blocks[i]);
>> break;
>>
>>
> Could the variable 'i' be declared as gl_shader_type to avoid all the
> casting?
>
> -Brian
>
I'm not thrilled about the casting either. I'm worried that some time in
the future someone will try removing one of the casts, see that there is no
compiler error, and come to the wrong conclusion that the cast does
nothing. (In reality, the cast is necessary because it's selecting between
two overloads of _mesa_glsl_shader_target_name(), one which takes a GLenum
and one which takes a gl_shader_type).
Unfortunatly, declaring i as gl_shader_type isn't a great fix because it
forces casting to be added elsewhere. The top of the "for" loop would have
to change to something like:
for (gl_shader_type i = (gl_shader_type) 0; i < MESA_SHADER_TYPES;
i = (gl_shader_type) (i + 1)) {
which seems pretty ugly.
How about this idea instead: rather than use function overloading to
distinguish between the two meanings of _mesa_glsl_shader_target_name(),
have two functions with separate names, e.g.:
const char *_mesa_gl_shader_type_to_string(unsigned): converts
MESA_SHADER_VERTEX -> "vertex", etc.
const char *_mesa_shader_target_enum_to_string(GLenum): converts
GL_VERTEX_SHADER -> "vertex", etc.
That would eliminate the need for typecasts, and it would reduce the danger
of someone calling the wrong function by mistake.
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131217/6a2d41b4/attachment.html>
More information about the mesa-dev
mailing list