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.engest...@imgtec.com> Cheers, Eric > > Could you clarify? > > Thanks! > -- > > Br, > > Andres _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev