On Tue, 2016-08-02 at 17:04 +0100, Eric Engestrom wrote:
> On Tue, Aug 02, 2016 at 12:20:54PM +0300, Andres Gomez wrote:
[snip]
> > 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>
Ouch, right!
Thanks for review and extended comments. I will apply your changes and
push.
Thanks!
--
Br,
Andres
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
