[Mesa-dev] [PATCH 3/9] glsl: Emit errors for things that look like default precision statements

[Kenneth Graunke]

On 08/09/2013 04:38 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.  We should instead generate an error.
>
> Fixes piglit precision-05.vert.
>
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> Cc: "9.2" <mesa-stable at lists.freedesktop.org>
> ---
>   src/glsl/ast_to_hir.cpp | 15 ++++++++-------
>   1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 49804b7..9d2ffff 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -2697,6 +2697,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.
> +       *
>          * 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
> @@ -2705,7 +2709,10 @@ 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 (this->type->qualifier.precision != ast_precision_none) {
> +         _mesa_glsl_error(&loc, state,
> +                          "set default precision using `precision' keyword");

I like how this message suggests what to do.  I don't like that it 
doesn't describe the actual problem.

Perhaps something like:
"empty declaration; perhaps you meant `precision highp %s'?", 
this->type->specifier->type_name

or:
"empty declaration; to set the default precision, use `precision highp 
%s;'", this->type->specifier->type_name

> +      } else if (this->type->specifier->structure == NULL) {
>   	 if (decl_type != NULL) {
>   	    _mesa_glsl_warning(&loc, state, "empty declaration");
>   	 } else {
> @@ -2714,12 +2721,6 @@ ast_declarator_list::hir(exec_list *instructions,
>   			     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");
> -      }
>      }
>
>      foreach_list_typed (ast_declaration, decl, link, &this->declarations) {

Although stupid, declarations like "highp float;" are actually permitted 
by the letter of the spec.  nVidia's driver also accepts them.  So I'm a 
bit uneasy about disallowing them.

We should check AMD.  If it disallows them, I'm fine with disallowing 
them as well.  If it accepts them, I think we should too (sadly).

I definitely approve of generating a better message, though.