[Mesa-dev] [PATCH 06/19] glsl: Refactor code to check that identifier names are valid.

[Kenneth Graunke]

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 <kenneth at 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
>