[Mesa-dev] [PATCH] ast: Updated AST_NUM_OPERATORS for coherence with ast_operators

[Eric Engestrom]

On Tue, Aug 02, 2016 at 12:20:54PM +0300, Andres Gomez wrote:
> On Mon, 2016-08-01 at 14:02 +0100, Eric Engestrom wrote:
> > On Sun, Jul 31, 2016 at 07:07:34PM +0300, Andres Gomez wrote:
> 
> [snip]
> 
> > > diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
> > > index 157895d..89f531c 100644
> > > --- a/src/compiler/glsl/ast.h
> > > +++ b/src/compiler/glsl/ast.h
> > > @@ -198,9 +198,19 @@ enum ast_operators {
> > >  
> > >     ast_sequence,
> > >     ast_aggregate
> > > +   /** Update AST_NUM_OPERATORS if more are appended */
> > >  };
> > >  
> > >  /**
> > > + * Number of possible operators for an ast_expression
> > > + *
> > > + * This is done as a define instead of as an additional value in the enum so
> > > + * that the compiler won't generate spurious messages like "warning:
> > > + * enumeration value ‘ast_num_operators’ not handled in switch"
> > > + */
> > > +#define AST_NUM_OPERATORS (ast_aggregate + 1)
> > 
> > Since you're moving the #define, why not move it at the end of the enum,
> > instead of adding the message there to go and look for the define?
> > Other than that, the change looks good :)
> 
> I'm not sure I'm understanding what you mean.
> 
> If you mean to remove the #define and add the value as and additional
> element to the enum, the existence of the define is precisely for not
> doing that, as explained in its comment.

I've read the comment, so this isn't what I was suggesting (I learned
something btw, it hadn't occurred to me adding the counter to the enum
would cause a warning, but I guess it makes sense).

> 
> If you mean to move the #define just below the enumeration, that's what
> this patch does.

What I meant is to move it to the end of the enum, not after it, so that
it becomes explicit that it's part of it (for humans, that is; it makes
no difference to the preprocessor). I expect it was originally just
after the enum, but with time things got inserted in between.

Something like this would be better IMHO:

8<--------------

diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
index 157895d..b5277fc 100644
--- a/src/compiler/glsl/ast.h
+++ b/src/compiler/glsl/ast.h
@@ -198,6 +198,15 @@ enum ast_operators {
 
    ast_sequence,
    ast_aggregate
+
+   /**
+    * Number of possible operators for an ast_expression
+    *
+    * This is done as a define instead of as an additional value in the enum so
+    * that the compiler won't generate spurious messages like "warning:
+    * enumeration value ‘ast_num_operators’ not handled in switch"
+    */
+   #define AST_NUM_OPERATORS (ast_aggregate + 1)
 };
 
 /**

8<--------------

It's not all that important, your change is good either way, so:
Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>

Cheers,
  Eric

> 
> Could you clarify?
> 
> Thanks!
> -- 
> 
> Br,
> 
> Andres