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