On 08/01/2013 08:07 AM, Paul Berry wrote:
On 31 July 2013 18:17, Ian Romanick <i...@freedesktop.org
<mailto:i...@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;
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
