[Mesa-dev] [PATCH] glsl: Generate compile errors for explicit blend indices < 0 or > 1.

[Paul Berry]

On 11 September 2012 00:40, Ian Romanick <idr at freedesktop.org> wrote:

> On 09/11/2012 03:06 AM, Paul Berry wrote:
>
>> On 31 August 2012 16:04, Kenneth Graunke <kenneth at whitecape.org
>> <mailto:kenneth at whitecape.org>**> wrote:
>>
>>     According to the GLSL 4.30 specification, this is a compile time
>> error.
>>     Earlier specifications don't specify a behavior, but since 0 and 1 are
>>     the only valid indices for dual source blending, it makes sense to
>>     generate the error.
>>
>>     Fixes (the fixed version of) piglit's layout-12.frag.
>>
>>     NOTE: This is a candidate for the 9.0 branch.
>>
>>     Signed-off-by: Kenneth Graunke <kenneth at whitecape.org
>>     <mailto:kenneth at whitecape.org>**>
>>
>>     ---
>>       src/glsl/ast_to_hir.cpp | 17 +++++++++++++++--
>>       1 file changed, 15 insertions(+), 2 deletions(-)
>>
>>     diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>>     index 02fe66b..becf6f9 100644
>>     --- a/src/glsl/ast_to_hir.cpp
>>     +++ b/src/glsl/ast_to_hir.cpp
>>     @@ -2086,9 +2086,22 @@ apply_type_qualifier_to_**variable(const
>> struct
>>     ast_type_qualifier *qual,
>>               } else {
>>                  var->location = qual->location;
>>               }
>>     +
>>               if (qual->flags.q.explicit_index) {
>>     -           var->explicit_index = true;
>>     -           var->index = qual->index;
>>     +            /* From the GLSL 4.30 specification: "It is also a
>>     compile-time
>>     +             * error if a fragment shader sets a layout index to
>>     less than 0
>>     +             * or greater than 1."
>>
>>
>> Minor nit pick: would you mind referencing the section of the spec this
>> quote comes from (4.4.2 Output Layout Qualifiers)?
>>
>
> I'll go a step further.  This should be formatted the way other spec
> references in the compiler are formatted:
>
>         /* Page XXX (page YYY of the PDF) of the GLSL A.BB spec says:
>          *
>          *     "Blah."
>          */
>
> It makes it a lot easier to find things using grep.
>

Ooh, I'd never thought of the grep advantages of formatting the quotes
consistently--I like that idea.  I would also like to be able to find the
quoted text in the spec itself easily (so I can see it in context).
 Unfortunately, it's often hard to do this based on the page number,
because the specs get revised without notice to make error corrections, and
this shifts the page numbers around.  Also, there are up to four versions
of some of the recent specs published in the registry (".core",
".core.withchanges", ".compatibility", and ".compatibility.withchanges"),
so if we just cite the page number it's ambiguous which document we are
referring to.

Can we adopt a standard way of formatting quotes that includes the section
number too?  Maybe something like

/* Page XXX (page YYY of the PDF) of GLSL A.BB (section S.T.P.Q) says:
 *
 *     "Blah."
 */

Note that with versions of the GL spec since 3.0, we can drop the "(page
YYY of the PDF)" part, because they finally figured out how to get the page
numbers in the pdf reader to match up between the page numbers printed on
the page.  Sadly, the GLSL specs still have inconsistent page numbering :(.


>
>  Other than that, this patch is:
>>
>> Reviewed-by: Paul Berry <stereotype441 at gmail.com
>> <mailto:stereotype441 at gmail.**com <stereotype441 at gmail.com>>>
>>
>
> and
>
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
>
>
>      +             *
>>     +             * Older specifications don't mandate a behavior; we
>>     take this
>>     +             * as a clarification and always generate the error.
>>     +             */
>>     +            if (qual->index < 0 || qual->index > 1) {
>>     +               _mesa_glsl_error(loc, state,
>>     +                                "explicit index may only be 0 or
>> 1\n");
>>     +            } else {
>>     +               var->explicit_index = true;
>>     +               var->index = qual->index;
>>     +            }
>>               }
>>             }
>>          } else if (qual->flags.q.explicit_index) {
>>     --
>>     1.7.11.4
>>
>> ______________________________**_________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/**mailman/listinfo/mesa-dev<http://lists.freedesktop.org/mailman/listinfo/mesa-dev>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120911/324e100c/attachment.html>