On 5 October 2013 12:12, Kenneth Graunke <kenn...@whitecape.org> wrote:
> 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()
>
Ok, I went with validate_identifier().
>
> > +{
> > + /* 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.
>
Ok, fixed.
>
> 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
