On 10/02/2013 05:45 PM, Paul Berry wrote:
> GLSL reserves identifiers beginning with "gl_" or containing "__", but
> we haven't been consistent about enforcing this rule. This patch
> makes a new function to check whether identifier names are valid. In
> the process it closes a loophole where we would previously allow
> function argument names to contain "__".
> ---
> src/glsl/ast_to_hir.cpp | 65
> ++++++++++++++++++++++++-------------------------
> 1 file changed, 32 insertions(+), 33 deletions(-)
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index c1e3c08..f4f81e0 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -2649,6 +2649,36 @@ handle_geometry_shader_input_decl(struct
> _mesa_glsl_parse_state *state,
> }
> }
>
> +
> +void
> +check_valid_identifier(const char *identifier, YYLTYPE loc,
> + struct _mesa_glsl_parse_state *state)
"check_valid_identifier" sounds strange if you read it aloud.
Perhaps one of the following?
- validate_identifier()
- check_for_valid_identifier()
- check_identifier_validity()
> +{
> + /* From page 15 (page 21 of the PDF) of the GLSL 1.10 spec,
> + *
> + * "Identifiers starting with "gl_" are reserved for use by
> + * OpenGL, and may not be declared in a shader as either a
> + * variable or a function."
> + */
> + if (strncmp(identifier, "gl_", 3) == 0)
I would really like to see curly braces here, for two reasons:
1. Even though it's a single statement, it spans multiple lines.
2. The else case uses curly braces.
Even if you don't take my suggestions, this is:
Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>
> + _mesa_glsl_error(&loc, state,
> + "identifier `%s' uses reserved `gl_' prefix",
> + identifier);
> + else if (strstr(identifier, "__")) {
> + /* From page 14 (page 20 of the PDF) of the GLSL 1.10
> + * spec:
> + *
> + * "In addition, all identifiers containing two
> + * consecutive underscores (__) are reserved as
> + * possible future keywords."
> + */
> + _mesa_glsl_error(&loc, state,
> + "identifier `%s' uses reserved `__' string",
> + identifier);
> + }
> +}
> +
> +
> ir_rvalue *
> ast_declarator_list::hir(exec_list *instructions,
> struct _mesa_glsl_parse_state *state)
> @@ -3243,28 +3273,7 @@ ast_declarator_list::hir(exec_list *instructions,
> * created for the declaration should be added to the IR stream.
> */
> if (earlier == NULL) {
> - /* From page 15 (page 21 of the PDF) of the GLSL 1.10 spec,
> - *
> - * "Identifiers starting with "gl_" are reserved for use by
> - * OpenGL, and may not be declared in a shader as either a
> - * variable or a function."
> - */
> - if (strncmp(decl->identifier, "gl_", 3) == 0)
> - _mesa_glsl_error(& loc, state,
> - "identifier `%s' uses reserved `gl_' prefix",
> - decl->identifier);
> - else if (strstr(decl->identifier, "__")) {
> - /* From page 14 (page 20 of the PDF) of the GLSL 1.10
> - * spec:
> - *
> - * "In addition, all identifiers containing two
> - * consecutive underscores (__) are reserved as
> - * possible future keywords."
> - */
> - _mesa_glsl_error(& loc, state,
> - "identifier `%s' uses reserved `__' string",
> - decl->identifier);
> - }
> + check_valid_identifier(decl->identifier, loc, state);
>
> /* Add the variable to the symbol table. Note that the initializer's
> * IR was already processed earlier (though it hasn't been emitted
> @@ -3505,17 +3514,7 @@ ast_function::hir(exec_list *instructions,
> "function body", name);
> }
>
> - /* From page 15 (page 21 of the PDF) of the GLSL 1.10 spec,
> - *
> - * "Identifiers starting with "gl_" are reserved for use by
> - * OpenGL, and may not be declared in a shader as either a
> - * variable or a function."
> - */
> - if (strncmp(name, "gl_", 3) == 0) {
> - YYLTYPE loc = this->get_location();
> - _mesa_glsl_error(&loc, state,
> - "identifier `%s' uses reserved `gl_' prefix", name);
> - }
> + check_valid_identifier(name, this->get_location(), state);
>
> /* Convert the list of function parameters to HIR now so that they can be
> * used below to compare this function's signature with previously seen
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
