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

[Ian Romanick]

On 08/01/2013 08:07 AM, Paul Berry wrote:
> On 31 July 2013 18:17, Ian Romanick <idr at freedesktop.org
> <mailto: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?

It has become such a habit, that I actually had to think about why it 
bugs me. :)  There are three reasons, I think.

- As you identified, the oddness of the empty if-statement in debug builds.

- It always makes me wonder if there used to be more code in the 
if-statement, and that makes me wonder if the assertion still being 
there is evidence of bit rot.

- Having the assertion be entirely self-contained makes it easier to 
copy-and-paste or cut-and-paste it elsewhere.

> 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:

That's fair.  I think in this case it's clearly more clear (ha!), so 
that outweighs my reasons.

I'll see if I can write something to put in the coding standards to 
capture both of our thoughts on this.  The bigger utility, I suspect, 
will be capturing the reasons behind the guidelines than capturing the 
guidelines themselves.

> /**
>   * 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?

Yes, that's exactly what I had in mind.

>     /**
>      * 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;