[Mesa-dev] [PATCH] glsl: Emit better warnings for things that look like default precision statements

[Kenneth Graunke]

On 08/13/2013 01:35 PM, Ian Romanick wrote:
> From: Ian Romanick <ian.d.romanick at intel.com>
>
> Previously we would emit a warning for empty declarations like
>
> float;
>
> We would also emit the same warning for things like
>
> highp float;
>
> However, this second case is most likely the application trying to set
> the default precision.  This makes the compiler generate a stronger
> warning with some suggestion of a fix.
>
> It really seems like this should be an error.  I'll bet that 100% of the
> time someone writes 'highp float;' the actually meant 'precision highp
> float;'.  Alas, both AMD and NVIDIA accept this syntax, and the spec
> doesn't explicitly forbid it.
>
> This makes piglit's precision-05.vert generate the following warnings:
>
> 0:12(11): warning: empty declaration with precision qualifier, to set the default precision, use `precision lowp float;'
> 0:13(12): warning: empty declaration with precision qualifier, to set the default precision, use `precision mediump int;'
>
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> Cc: Kenneth Graunke <kenneth at whitecape.org>
> Cc: "9.2" <mesa-stable at lists.freedesktop.org>
> ---
>   src/glsl/ast_to_hir.cpp | 43 ++++++++++++++++++++++++++++++-------------
>   1 file changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 40992fb..f96b64b 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -2719,6 +2719,10 @@ ast_declarator_list::hir(exec_list *instructions,
>          *   name of a known structure type.  This is both invalid and weird.
>          *   Emit an error.
>          *
> +       * - The program text contained something like 'mediump float;'
> +       *   when the programmer probably meant 'precision mediump
> +       *   float;' Emit an error.

You mean "Emit a warning" here.

> +       *
>          * Note that if decl_type is NULL and there is a structure involved,
>          * there must have been some sort of error with the structure.  In this
>          * case we assume that an error was already generated on this line of
> @@ -2727,20 +2731,33 @@ ast_declarator_list::hir(exec_list *instructions,
>          */
>         assert(this->type->specifier->structure == NULL || decl_type != NULL
>   	     || state->error);
> -      if (this->type->specifier->structure == NULL) {
> -	 if (decl_type != NULL) {
> -	    _mesa_glsl_warning(&loc, state, "empty declaration");
> -	 } else {
> -	    _mesa_glsl_error(&loc, state,
> -			     "invalid type `%s' in empty declaration",
> -			     type_name);
> -	 }
> -      }
>
> -      if (this->type->qualifier.precision != ast_precision_none &&
> -          this->type->specifier->structure != NULL) {
> -         _mesa_glsl_error(&loc, state, "precision qualifiers can't be applied "
> -                          "to structures");
> +      if (decl_type == NULL) {
> +         _mesa_glsl_error(&loc, state,
> +                          "invalid type `%s' in empty declaration",
> +                          type_name);
> +      } else if (this->type->qualifier.precision != ast_precision_none) {
> +         if (this->type->specifier->structure != NULL)
> +            _mesa_glsl_error(&loc, state,
> +                             "precision qualifiers can't be applied "
> +                             "to structures");
> +         else {

Please use curly braces around the first branch as well.  Yes, it's only 
one statement, but it's three lines, and adding them doesn't take any 
additional space.

Other than those minor nits, this is
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

> +            static const char *const precision_names[] = {
> +               "highp",
> +               "highp",
> +               "mediump",
> +               "lowp"
> +            };
> +
> +            _mesa_glsl_warning(&loc, state,
> +                               "empty declaration with precision qualifier, "
> +                               "to set the default precision, use "
> +                               "`precision %s %s;'",
> +                               precision_names[this->type->qualifier.precision],
> +                               type_name);
> +         }
> +      } else {
> +         _mesa_glsl_warning(&loc, state, "empty declaration");
>         }
>      }
>
>