<div dir="ltr">On 31 July 2013 18:17, Ian Romanick <span dir="ltr"><<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class=""><div class="h5">On 07/28/2013 11:03 PM, Paul Berry wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
---<br>
  src/glsl/ast.h          | 24 ++++++++++++++++--------<br>
  src/glsl/ast_to_hir.cpp | 31 +++++++++++++++++++++++++++++-<u></u>-<br>
  src/glsl/glsl_parser.yy | 11 ++++-------<br>
  3 files changed, 49 insertions(+), 17 deletions(-)<br>
<br>
diff --git a/src/glsl/ast.h b/src/glsl/ast.h<br>
index 3ef9913..a40bbc0 100644<br>
--- a/src/glsl/ast.h<br>
+++ b/src/glsl/ast.h<br>
@@ -907,12 +907,14 @@ public:<br>
  class ast_interface_block : public ast_node {<br>
  public:<br>
     ast_interface_block(ast_type_<u></u>qualifier layout,<br>
-                     const char *instance_name,<br>
-                    ast_expression *array_size)<br>
+                       const char *instance_name,<br>
+                       bool is_array,<br>
+                       ast_expression *array_size)<br>
     : layout(layout), block_name(NULL), instance_name(instance_name),<br>
-     array_size(array_size)<br>
+     is_array(is_array), array_size(array_size)<br>
     {<br>
-      /* empty */<br>
+      if (!is_array)<br>
+         assert(array_size == NULL);<br>
</blockquote>
<br></div></div>
I think this is better as:<br>
<br>
        assert(is_array || array_size == NULL);<br>
<br>
The otherwise empty if-statements always bug me.</blockquote><div><br></div><div>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?<br>
<br></div><div>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:<br>
<br>/**<br> * If the block is not declared as an array or if the block instance<br> * array is unsizied, this field will be \c NULL.<br> */<br><br></div><div>to:<br><br></div><div>if(!is_array) assert(array_size == NULL);<br>
<br></div><div>than I do translating to:<br><br></div><div>assert(is_array || array_size == NULL);</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
     }<br>
<br>
     virtual ir_rvalue *hir(exec_list *instructions,<br>
@@ -933,13 +935,19 @@ public:<br>
     exec_list declarations;<br>
<br>
     /**<br>
-    * Declared array size of the block instance<br>
-    *<br>
-    * If the block is not declared as an array, this field will be \c NULL.<br>
+    * True if the block is declared as an array<br>
      *<br>
      * \note<br>
      * A block can only be an array if it also has an instance name.  If this<br>
-    * field is not \c NULL, ::instance_name must also not be \c NULL.<br>
+    * field is true, ::instance_name must also not be \c NULL.<br>
+    */<br>
+   bool is_array;<br>
+<br>
+   /**<br>
+    * Declared array size of the block instance, or NULL if the block instance<br>
+    * array is unsized.<br>
</blockquote>
<br></div>
I'd leave the first line as-is and change the second part to be:<br>
<br>
   * If the block is not declared as an array or if the block instance<br>
   * array is unsizied, this field will be \c NULL.<br>
<br>
That has the added benefit of making the diff look cleaner.</blockquote><div><br></div><div>I'm not certain I'm following your suggestion.  Is this better?<br><br>   /**<br>    * True if the block is declared as an array<br>
    *<br>    * \note<br>    * A block can only be an array if it also has an instance name.  If this<br>    * field is true, ::instance_name must also not be \c NULL.<br>    */<br>   bool is_array;<br><br>   /**<br>    * Declared array size of the block instance<br>
    *<br>    * If the block is not declared as an array or if the block instance array<br>    * is unsized, this field will be \c NULL.<br>    */<br>   ast_expression *array_size;<br> <br></div></div></div></div>