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

[Paul Berry]

On 5 October 2013 12:12, Kenneth Graunke <kenneth at 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 <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
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131008/f214b84d/attachment.html>