[Mesa-dev] [PATCH 05/13] glsl: simplifies the merge of the default in layout qualifier

[Andres Gomez]

On Wed, 2016-11-23 at 08:46 +1100, Timothy Arceri wrote:
> On Tue, 2016-11-22 at 16:07 +0200, Andres Gomez wrote:
> > On Tue, 2016-11-22 at 22:22 +1100, Timothy Arceri wrote:
> > ...
> > > 
> > > 
> > > Can I ask that you write a follow up patch for this series that
> > > creates
> > > a helper function for each of these validations. For example:
> > > 
> > > static bool
> > > validate_ordering(loc, state, qualifier, new_qualifier)
> > > {
> > >    if (qualifier->flags.q.ordering && new_qualifier-
> > > > flags.q.ordering
> > > 
> > >        && qualifier->ordering != new_qualifier->ordering) {
> > >       _mesa_glsl_error(loc, state,
> > >                        "conflicting ordering specified");
> > >    }
> > > 
> > >    return true;
> > > }
> > > 
> > > In merge_qualifier() just put the call in the outer if. For
> > > example:
> > > 
> > > if (q.flags.q.ordering && validate_ordering(...))
> > > 
> > > This will be better IMO as the code currently exists early when
> > > there
> > > is no reason we shouldn't continue on and check the other layout
> > > qualifiers before reporting errors.
> > 
> > 
> > OK, I will continue with this follow up but I don't think I can just
> > put the check in the outer if since, then, I will modify the
> > returning
> > value of the function in some cases. I will work on that.
> 
> 
> _mesa_glsl_error() will set state->error to true so you should be able
> to use that at the end of merge_qualifier()

I thought of it and that approach can be used quite broadly everywhere
else in the parser but it is not nice.

I know that may be far fetched but checking for state->error implies
that the implementation of the caller relies on the actual
implementation of the method called. In other words, it will work
because we assume a specific implementation of the function being
called instead of seeing it as a black box that will just return true
or false. If that implementation changes in the future we may have
undesired and unexpected collateral misbehaviors.

I will just rely on the value returned.

Anyway, running the tests right now. I will send the new series
tomorrow morning.

-- 
Br,

Andres