[Mesa-dev] [PATCH] glsl: remove excess location qualifier validation

[Timothy Arceri]

On Fri, 2015-10-23 at 08:01 +0300, Tapani Pälli wrote:
> On 10/22/2015 11:07 AM, Timothy Arceri wrote:
> > On Thu, 2015-10-22 at 09:56 +0300, Tapani Pälli wrote:
> > > On 10/22/2015 09:41 AM, Timothy Arceri wrote:
> > > > On Thu, 2015-10-22 at 08:55 +0300, Tapani Pälli wrote:
> > > > > On 10/22/2015 08:29 AM, Timothy Arceri wrote:
> > > > > > Location has never been able to be a negative value because
> > > > > > it
> > > > > > has
> > > > > > always been validated in the parser.
> > > > > > 
> > > > > > Also the linker doesn't check for negatives like the
> > > > > > comment
> > > > > > claims.
> > > > > Neither does the parser, if one utilizes negative explicit
> > > > > location,
> > > > > parser says:
> > > > > 
> > > > > error: syntax error, unexpected '-', expecting INTCONSTANT or
> > > > > UINTCONSTANT
> > > > > 
> > > > > I'm not sure if this is quite OK, it should rather accept the
> > > > > negative
> > > > > value and the fail here in this check you are about to
> > > > > remove?
> > > > Yes I did noticed this. However even if we changed it to accept
> > > > a
> > > > negative value the is another check in the parser that would
> > > > catch
> > > > this
> > > > before the checks I'm removing here.
> > > > 
> > > > if ($3 >= 0) {
> > > >      $$.location = $3;
> > > > } else {
> > > >       _mesa_glsl_error(& @3, state, "invalid location %d
> > > > specified",
> > > > $3);
> > > >                YYERROR;
> > > > }
> > > OK, then removing the check for uniform loc should be fine. For
> > > the
> > > attributes change, I'm not sure yet, I believe the reason for the
> > > 'silliness' (mentioned in the commit) is that all the built-in
> > > attributes have negative locations, we need to be careful to
> > > continue
> > > dealing gracefully with that.
> > This function is only valdating the qualifier, builtins don't seem
> > to
> > set the qualifier flags in the ast, as far as I can tell they set
> > the
> > fields in the ir directly.
> > 
> > For location the only place that sets the explicit location flag is
> > the
> > parser and this validation function is only called when that is
> > set, so
> > I don't believe this should be a problem.
> > 
> >     if (qual->flags.q.explicit_location) {
> >        validate_explicit_location(qual, var, state, loc);
> >     }
> 
> Yep, this is the case. Builtin variables have explicit locations but 
> those are set directly to the ir_variable when creating them. I can't
> find any way of them being negative here (especially as parser just
> hits 
> syntax error with both attributes and uniforms for locations < 0):

Great! Thanks for the review.

> 
> Reviewed-by: Tapani Pälli <tapani.palli at intel.com>
>