[Mesa-dev] [PATCH 2/5] glsl: Get rid of hardcoded arrays of shader target names.
Brian Paul
brianp at vmware.com
Tue Dec 17 11:50:39 PST 2013
On 12/17/2013 12:28 PM, Paul Berry wrote:
> On 17 December 2013 10:51, Brian Paul <brianp at vmware.com
> <mailto: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.
Yeah. I didn't realize C++ was pickier than C in this area.
> 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.
That sounds fine. Though I think slightly shorter functions names could
be used:
_mesa_shader_type_to_string(), _mesa_shader_enum_to_string()
But no big deal.
-Brian
More information about the mesa-dev
mailing list