[Mesa-dev] [PATCH 32/34] glsl: Allow geometry shader input instance arrays to be unsized.

[Paul Berry]

On 31 July 2013 18:17, Ian Romanick <idr at freedesktop.org> wrote:

> On 07/28/2013 11:03 PM, Paul Berry wrote:
>
>> ---
>>   src/glsl/ast.h          | 24 ++++++++++++++++--------
>>   src/glsl/ast_to_hir.cpp | 31 +++++++++++++++++++++++++++++-**-
>>   src/glsl/glsl_parser.yy | 11 ++++-------
>>   3 files changed, 49 insertions(+), 17 deletions(-)
>>
>> diff --git a/src/glsl/ast.h b/src/glsl/ast.h
>> index 3ef9913..a40bbc0 100644
>> --- a/src/glsl/ast.h
>> +++ b/src/glsl/ast.h
>> @@ -907,12 +907,14 @@ public:
>>   class ast_interface_block : public ast_node {
>>   public:
>>      ast_interface_block(ast_type_**qualifier layout,
>> -                     const char *instance_name,
>> -                    ast_expression *array_size)
>> +                       const char *instance_name,
>> +                       bool is_array,
>> +                       ast_expression *array_size)
>>      : layout(layout), block_name(NULL), instance_name(instance_name),
>> -     array_size(array_size)
>> +     is_array(is_array), array_size(array_size)
>>      {
>> -      /* empty */
>> +      if (!is_array)
>> +         assert(array_size == NULL);
>>
>
> I think this is better as:
>
>         assert(is_array || array_size == NULL);
>
> The otherwise empty if-statements always bug me.


I won't press you on the issue, but since you've asked me to rephrase
assertions like this before, I'm curious why my original formulation bugs
you.  Are you bothered because it makes it less obvious that the check will
compile down to nothing in release builds?

The reason I personally prefer the approach with the "if" statement is
because it more closely matches the way we talk about these invariants
using if/then language in the comments.  I have an easier time mentally
translating from:

/**
 * If the block is not declared as an array or if the block instance
 * array is unsizied, this field will be \c NULL.
 */

to:

if(!is_array) assert(array_size == NULL);

than I do translating to:

assert(is_array || array_size == NULL);


>
>
>       }
>>
>>      virtual ir_rvalue *hir(exec_list *instructions,
>> @@ -933,13 +935,19 @@ public:
>>      exec_list declarations;
>>
>>      /**
>> -    * Declared array size of the block instance
>> -    *
>> -    * If the block is not declared as an array, this field will be \c
>> NULL.
>> +    * True if the block is declared as an array
>>       *
>>       * \note
>>       * A block can only be an array if it also has an instance name.  If
>> this
>> -    * field is not \c NULL, ::instance_name must also not be \c NULL.
>> +    * field is true, ::instance_name must also not be \c NULL.
>> +    */
>> +   bool is_array;
>> +
>> +   /**
>> +    * Declared array size of the block instance, or NULL if the block
>> instance
>> +    * array is unsized.
>>
>
> I'd leave the first line as-is and change the second part to be:
>
>    * If the block is not declared as an array or if the block instance
>    * array is unsizied, this field will be \c NULL.
>
> That has the added benefit of making the diff look cleaner.


I'm not certain I'm following your suggestion.  Is this better?

   /**
    * True if the block is declared as an array
    *
    * \note
    * A block can only be an array if it also has an instance name.  If this
    * field is true, ::instance_name must also not be \c NULL.
    */
   bool is_array;

   /**
    * Declared array size of the block instance
    *
    * If the block is not declared as an array or if the block instance
array
    * is unsized, this field will be \c NULL.
    */
   ast_expression *array_size;
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130801/be0b3d06/attachment-0001.html>