util: Add array_index support to [vertex data]

Submitted by Andres Gomez on April 25, 2016, 1:56 p.m.

Details

Message ID 1461592561-23003-1-git-send-email-agomez@igalia.com
State Superseded
Headers show
Series "util: Add array_index support to [vertex data]" ( rev: 1 ) in Piglit

Not browsing as part of any series.

Commit Message

Andres Gomez April 25, 2016, 1:56 p.m.
This allows data to be set for arbitrary array sized attributes in
shader runner.

For example to set mat2x3[2]:
attname[0]/mat2x3/3/1 attname[0]/mat2x3/3/2 attname[1]/mat2x3/3/1 attname[1]/mat2x3/3/2

The syntax has been extended so the recommended type to specify in the
[vertex data] header line is the GLSL corresponding one, not just
"float", "double", "int" or "uint".

In any case, the extended syntax is backward compatible so it is still
possible to specify a vec3 attribute like:
attname/float/3

In addition to the now recommended format:
attname/vec3/3

Signed-off-by: Andres Gomez <agomez@igalia.com>
---
 tests/util/piglit-vbo.cpp | 154 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 118 insertions(+), 36 deletions(-)

Patch hide | download patch | download mbox

diff --git a/tests/util/piglit-vbo.cpp b/tests/util/piglit-vbo.cpp
index 11a4adc..749a1ed 100644
--- a/tests/util/piglit-vbo.cpp
+++ b/tests/util/piglit-vbo.cpp
@@ -1,5 +1,5 @@ 
 /*
- * Copyright © 2011 Intel Corporation
+ * Copyright © 2011, 2016 Intel Corporation
  *
  * Permission is hereby granted, free of charge, to any person obtaining a
  * copy of this software and associated documentation files (the "Software"),
@@ -28,29 +28,33 @@ 
  * tests using a columnar text format, for example:
  *
  *   \verbatim
- *   vertex/float/3 foo/uint/1 bar/int/2
- *   0.0 0.0 0.0    10         0 0 # comment
- *   0.0 1.0 0.0     5         1 1
- *   1.0 1.0 0.0     0         0 1
+ *   vertex/vec3/3 foo/uint/1 bar[0]/int/1 bar[1]/int/1
+ *   0.0 0.0 0.0   10         0            0 # comment
+ *   0.0 1.0 0.0    5         1            1
+ *   1.0 1.0 0.0    0         0            1
  *   \endverbatim
  *
  * The format consists of a row of column headers followed by any
  * number of rows of data.  Each column header has the form
- * "ATTRNAME/TYPE/COUNT/MATRIX_COLUMN", where ATTRNAME is the name of
- * the vertex attribute to be bound to this column, TYPE is the type
- * of data that follows ("float", "int", or "uint"), COUNT is the
- * vector length of the data (e.g. "3" for vec3 data) and
- * MATRIX_COLUMN is the column number of the data in case of being a
- * matrix. MATRIX_COLUMN can also be used for arrays of
- * arrays. MATRIX_COLUMN doesn't need to be be specified if it is not
- * a matrix column as in the example before. So another example, if
- * you want to specify the data of a mat2x3:
+ * "ATTRNAME[ARRAY_INDEX]/TYPE/COUNT/MATRIX_COLUMN", where ATTRNAME is
+ * the name of the vertex attribute to be bound to this column,
+ * ARRAY_INDEX is the index, TYPE is the GLSL type of data that
+ * follows ("float", "double", "int", "uint" or any derived type like
+ * vec2, dmat4, etc.), COUNT is the vector length of the data
+ * (e.g. "3" for vec3 data or "2" for mat4x2) and MATRIX_COLUMN is the
+ * column number of the data in case of being a matrix. [ARRAY_INDEX]
+ * is optional and needs to be specified only in case of array
+ * attributes. COUNT is a redundant value that is already specified in
+ * the data type but that has been kept for backward
+ * compatibility. MATRIX_COLUMN doesn't need to be specified if it is
+ * not a matrix column as in the example before. So another example,
+ * if you want to specify the data of a mat2x3:
  *
- *  \verbatim
- *  foomatrix/float/3/0 foomatrix/float/3/1
- *  0.0 1.0 2.0         3.0 4.0 5.0
- *  6.0 7.0 8.0         9.0 10.0 11.0
- *  \endverbatim
+ *   \verbatim
+ *   foomatrix/mat2x3/3/0 foomatrix/mat2x3/3/1
+ *   0.0 1.0 2.0          3.0 4.0 5.0
+ *   6.0 7.0 8.0          9.0 10.0 11.0
+ *   \endverbatim
  *
  * The data follows the column headers in space-separated form.  "#"
  * can be used for comments, as in shell scripts.
@@ -95,6 +99,23 @@ 
  * glEnableVertexAttribArray(foo_index);
  * glEnableVertexAttribArray(bar_index);
  * \endcode
+ *
+ * Which could correspond to a vertex shader of this type:
+ *
+ * \code
+ * in vec3 vertex;
+ * in uint foo;
+ * in int bar[2];
+ *
+ * void main()
+ * {
+ *  if (bar[0] + bar[1] == foo) {
+ *   gl_Position = vec4(vertex, 1.0);
+ *  } else {
+ *   gl_Position = vec4(1.0, vertex);
+ *  }
+ * }
+ * \endcode
  */
 
 #include <string>
@@ -116,7 +137,7 @@  const int ATTRIBUTE_SIZE = 4;
  * Convert a type name string to a GLenum.
  */
 GLenum
-decode_type(const char *type)
+decode_type(const char *type, size_t *rows, size_t *cols)
 {
 	static struct type_table_entry {
 		const char *type; /* NULL means end of table */
@@ -126,13 +147,38 @@  decode_type(const char *type)
 		{ "uint",    GL_UNSIGNED_INT   },
 		{ "float",   GL_FLOAT          },
 		{ "double",  GL_DOUBLE         },
+		{ "ivec",    GL_INT            },
+		{ "uvec",    GL_UNSIGNED_INT   },
+		{ "vec",     GL_FLOAT          },
+		{ "dvec",    GL_DOUBLE         },
+		{ "mat",     GL_FLOAT          },
+		{ "dmat",    GL_DOUBLE         },
 		{ NULL,      0                 }
 	};
 
 
 	for (int i = 0; type_table[i].type; ++i) {
-		if (0 == strcmp(type, type_table[i].type))
+		if (0 == strncmp(type, type_table[i].type, strlen(type_table[i].type))) {
+			if (i > 3 && (rows || cols)) {
+				size_t type_len = strlen(type);
+				size_t the_rows = type[type_len - 1] - '0';
+				if (rows)
+					*rows = the_rows;
+				if (cols) {
+					if (i > 7)
+						*cols = type[type_len - 2] == 'x' ?
+							type[type_len - 3] - '0' : the_rows;
+					else
+						*cols = 1;
+				}
+			} else {
+				if (rows)
+					*rows = 1;
+				if (cols)
+					*cols = 1;
+			}
 			return type_table[i].enum_value;
+		}
 	}
 
 	printf("Unrecognized type: %s\n", type);
@@ -157,6 +203,11 @@  public:
 	GLenum data_type;
 
 	/**
+	 * Number of cols in the data type of this attribute.
+	 */
+	size_t data_type_cols;
+
+	/**
 	 * Vector length of this attribute.
 	 */
 	size_t count;
@@ -167,6 +218,11 @@  public:
 	size_t matrix_index;
 
 	/**
+	 * Index of the array for this attribute.
+	 */
+	size_t array_index;
+
+	/**
 	 * Index of this vertex attribute in the linked program.
 	 */
 	GLuint index;
@@ -184,25 +240,49 @@  public:
 vertex_attrib_description::vertex_attrib_description(GLuint prog,
 						     const char *text)
 {
-	/* Split the column header into name/type/count fields */
+	/* Split the column header into
+	 * name[array_index]/type/count/matrix_column fields */
 	const char *first_slash = strchr(text, '/');
 	if (first_slash == NULL) {
-		printf("Column headers must be in the form name/type/count/matrix_column.\n"
-		       "Note: matrix_column is optional.\n"
+		printf("Column headers must be in the form"
+		       " name[array_index]/type/count/matrix_column.\n"
+		       "Note: [array_index] and matrix_column are optional.\n"
 		       "Got: %s\n",
 		       text);
 		piglit_report_result(PIGLIT_FAIL);
 	}
+
 	std::string name(text, first_slash);
+
+	/* If the attrib is an array, strip the index */
+	if (name[name.size() - 1] == ']') {
+		std::string::size_type n = name.find('[');
+		if (n == std::string::npos) {
+			printf("Column header looked like an array"
+			       " but couldn't parse it.\n"
+			       "Got: %s\n",
+			       text);
+			piglit_report_result(PIGLIT_FAIL);
+		} else {
+			this->array_index = strtoul(name.substr(n + 1).c_str(), NULL, 0);
+			name.resize(n);
+		}
+	} else {
+		this->array_index = 0;
+	}
+
 	const char *second_slash = strchr(first_slash + 1, '/');
 	if (second_slash == NULL) {
-		printf("Column headers must be in the form name/type/count/matrix_column.\n"
+		printf("Column headers must be in the form"
+		       " name[array_index]/type/count/matrix_column.\n"
+		       "Note: [array_index] and matrix_column are optional.\n"
 		       "Got: %s\n",
 		       text);
 		piglit_report_result(PIGLIT_FAIL);
 	}
 	std::string type_str(first_slash + 1, second_slash);
-	this->data_type = decode_type(type_str.c_str());
+	this->data_type = decode_type(type_str.c_str(),
+				      &this->count, &this->data_type_cols);
 	char *endptr;
 	this->count = strtoul(second_slash + 1, &endptr, 10);
 
@@ -217,8 +297,9 @@  vertex_attrib_description::vertex_attrib_description(GLuint prog,
 		}
 
 		if (*endptr != '\0') {
-			printf("Column headers must be in the form name/type/matrix_column.\n"
-			       "Note: matrix_column is optional.\n"
+			printf("Column headers must be in the form"
+			       " name[array_index]/type/count/matrix_column.\n"
+			       "Note: [array_index] and matrix_column are optional.\n"
 			       "Got: %s\n",
 			       text);
 			piglit_report_result(PIGLIT_FAIL);
@@ -323,9 +404,11 @@  void
 vertex_attrib_description::setup(size_t *offset, size_t stride) const
 {
 	int attribute_size = ATTRIBUTE_SIZE;
+	GLuint actual_index = this->index + this->matrix_index
+		+ this->array_index * this->data_type_cols;
 	switch (this->data_type) {
 	case GL_FLOAT:
-		glVertexAttribPointer(this->index + this->matrix_index, this->count,
+		glVertexAttribPointer(actual_index, this->count,
 				      this->data_type, GL_FALSE, stride,
 				      (void *) *offset);
 		break;
@@ -334,9 +417,9 @@  vertex_attrib_description::setup(size_t *offset, size_t stride) const
 			fprintf(stderr,"vertex_attrib_description fail. no 64-bit float support\n");
 			return;
 		}
-		glVertexAttribLPointer(this->index + this->matrix_index, this->count,
-				      this->data_type, stride,
-				      (void *) *offset);
+		glVertexAttribLPointer(actual_index, this->count,
+				       this->data_type, stride,
+				       (void *) *offset);
 		attribute_size *= 2;
 		break;
 	default:
@@ -344,12 +427,11 @@  vertex_attrib_description::setup(size_t *offset, size_t stride) const
 			fprintf(stderr,"vertex_attrib_description fail. no int support\n");
 			return;
 		}
-		glVertexAttribIPointer(this->index + this->matrix_index, this->count,
+		glVertexAttribIPointer(actual_index, this->count,
 				       this->data_type, stride,
-				       (void *) *offset);
-		break;
+				       (void *) *offset);		break;
 	}
-	glEnableVertexAttribArray(index + this->matrix_index);
+	glEnableVertexAttribArray(actual_index);
 	*offset += attribute_size * this->count;
 }
 

Comments

On 25/04/16 15:56, Andres Gomez wrote:
> This allows data to be set for arbitrary array sized attributes in
> shader runner.
>
> For example to set mat2x3[2]:
> attname[0]/mat2x3/3/1 attname[0]/mat2x3/3/2 attname[1]/mat2x3/3/1 attname[1]/mat2x3/3/2
>
> The syntax has been extended so the recommended type to specify in the
> [vertex data] header line is the GLSL corresponding one, not just
> "float", "double", "int" or "uint".
>
> In any case, the extended syntax is backward compatible so it is still
> possible to specify a vec3 attribute like:
> attname/float/3
>
> In addition to the now recommended format:
> attname/vec3/3

Nitpick: Are you sure that is the recommended instead of just the
extended one?

>
> Signed-off-by: Andres Gomez <agomez@igalia.com>
> ---
>  tests/util/piglit-vbo.cpp | 154 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 118 insertions(+), 36 deletions(-)
>
> diff --git a/tests/util/piglit-vbo.cpp b/tests/util/piglit-vbo.cpp
> index 11a4adc..749a1ed 100644
> --- a/tests/util/piglit-vbo.cpp
> +++ b/tests/util/piglit-vbo.cpp
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright © 2011 Intel Corporation
> + * Copyright © 2011, 2016 Intel Corporation
>   *
>   * Permission is hereby granted, free of charge, to any person obtaining a
>   * copy of this software and associated documentation files (the "Software"),
> @@ -28,29 +28,33 @@
>   * tests using a columnar text format, for example:
>   *
>   *   \verbatim
> - *   vertex/float/3 foo/uint/1 bar/int/2
> - *   0.0 0.0 0.0    10         0 0 # comment
> - *   0.0 1.0 0.0     5         1 1
> - *   1.0 1.0 0.0     0         0 1
> + *   vertex/vec3/3 foo/uint/1 bar[0]/int/1 bar[1]/int/1
> + *   0.0 0.0 0.0   10         0            0 # comment
> + *   0.0 1.0 0.0    5         1            1
> + *   1.0 1.0 0.0    0         0            1
>   *   \endverbatim
>   *
>   * The format consists of a row of column headers followed by any
>   * number of rows of data.  Each column header has the form
> - * "ATTRNAME/TYPE/COUNT/MATRIX_COLUMN", where ATTRNAME is the name of
> - * the vertex attribute to be bound to this column, TYPE is the type
> - * of data that follows ("float", "int", or "uint"), COUNT is the
> - * vector length of the data (e.g. "3" for vec3 data) and
> - * MATRIX_COLUMN is the column number of the data in case of being a
> - * matrix. MATRIX_COLUMN can also be used for arrays of
> - * arrays. MATRIX_COLUMN doesn't need to be be specified if it is not
> - * a matrix column as in the example before. So another example, if
> - * you want to specify the data of a mat2x3:
> + * "ATTRNAME[ARRAY_INDEX]/TYPE/COUNT/MATRIX_COLUMN", where ATTRNAME is
> + * the name of the vertex attribute to be bound to this column,
> + * ARRAY_INDEX is the index, TYPE is the GLSL type of data that
> + * follows ("float", "double", "int", "uint" or any derived type like
> + * vec2, dmat4, etc.), COUNT is the vector length of the data
> + * (e.g. "3" for vec3 data or "2" for mat4x2) and MATRIX_COLUMN is the
> + * column number of the data in case of being a matrix. [ARRAY_INDEX]
> + * is optional and needs to be specified only in case of array
> + * attributes. COUNT is a redundant value that is already specified in
> + * the data type but that has been kept for backward
> + * compatibility.

Now that you mention backward compatibility, I think that it would be
good to include some examples with the old format. After all, is the
format of most tests right now.

>  MATRIX_COLUMN doesn't need to be specified if it is
> + * not a matrix column as in the example before. So another example,
> + * if you want to specify the data of a mat2x3:
>   *
> - *  \verbatim
> - *  foomatrix/float/3/0 foomatrix/float/3/1
> - *  0.0 1.0 2.0         3.0 4.0 5.0
> - *  6.0 7.0 8.0         9.0 10.0 11.0
> - *  \endverbatim
> + *   \verbatim
> + *   foomatrix/mat2x3/3/0 foomatrix/mat2x3/3/1
> + *   0.0 1.0 2.0          3.0 4.0 5.0
> + *   6.0 7.0 8.0          9.0 10.0 11.0
> + *   \endverbatim
>   *
>   * The data follows the column headers in space-separated form.  "#"
>   * can be used for comments, as in shell scripts.
> @@ -95,6 +99,23 @@
>   * glEnableVertexAttribArray(foo_index);
>   * glEnableVertexAttribArray(bar_index);
>   * \endcode
> + *
> + * Which could correspond to a vertex shader of this type:
> + *
> + * \code
> + * in vec3 vertex;
> + * in uint foo;
> + * in int bar[2];
> + *
> + * void main()
> + * {
> + *  if (bar[0] + bar[1] == foo) {
> + *   gl_Position = vec4(vertex, 1.0);
> + *  } else {
> + *   gl_Position = vec4(1.0, vertex);
> + *  }
> + * }
> + * \endcode
>   */
>  
>  #include <string>
> @@ -116,7 +137,7 @@ const int ATTRIBUTE_SIZE = 4;
>   * Convert a type name string to a GLenum.
>   */
>  GLenum
> -decode_type(const char *type)
> +decode_type(const char *type, size_t *rows, size_t *cols)
>  {
>  	static struct type_table_entry {
>  		const char *type; /* NULL means end of table */
> @@ -126,13 +147,38 @@ decode_type(const char *type)
>  		{ "uint",    GL_UNSIGNED_INT   },
>  		{ "float",   GL_FLOAT          },
>  		{ "double",  GL_DOUBLE         },
> +		{ "ivec",    GL_INT            },
> +		{ "uvec",    GL_UNSIGNED_INT   },
> +		{ "vec",     GL_FLOAT          },
> +		{ "dvec",    GL_DOUBLE         },
> +		{ "mat",     GL_FLOAT          },
> +		{ "dmat",    GL_DOUBLE         },
>  		{ NULL,      0                 }
>  	};
>  
>  
>  	for (int i = 0; type_table[i].type; ++i) {
> -		if (0 == strcmp(type, type_table[i].type))
> +		if (0 == strncmp(type, type_table[i].type, strlen(type_table[i].type))) {
> +			if (i > 3 && (rows || cols)) 
What 3 is lacks somewhat a context, as you need to go back to type_table
to know what you want to do. Couldn't it be replaced by a kind of enum
or at least a comment?

> +				size_t type_len = strlen(type);
> +				size_t the_rows = type[type_len - 1] - '0';
> +				if (rows)
> +					*rows = the_rows;
> +				if (cols) {
> +					if (i > 7)

Ditto

> +						*cols = type[type_len - 2] == 'x' ?
> +							type[type_len - 3] - '0' : the_rows;
> +					else
> +						*cols = 1;
> +				}
> +			} else {
> +				if (rows)
> +					*rows = 1;
> +				if (cols)
> +					*cols = 1;
> +			}
>  			return type_table[i].enum_value;
> +		}
>  	}
>  
>  	printf("Unrecognized type: %s\n", type);
> @@ -157,6 +203,11 @@ public:
>  	GLenum data_type;
>  
>  	/**
> +	 * Number of cols in the data type of this attribute.

For the comment s/cols/columns

> +	 */
> +	size_t data_type_cols;
> +
> +	/**
>  	 * Vector length of this attribute.
>  	 */
>  	size_t count;
> @@ -167,6 +218,11 @@ public:
>  	size_t matrix_index;
>  
>  	/**
> +	 * Index of the array for this attribute.
> +	 */
> +	size_t array_index;
> +
> +	/**
>  	 * Index of this vertex attribute in the linked program.
>  	 */
>  	GLuint index;
> @@ -184,25 +240,49 @@ public:
>  vertex_attrib_description::vertex_attrib_description(GLuint prog,
>  						     const char *text)
>  {
> -	/* Split the column header into name/type/count fields */
> +	/* Split the column header into
> +	 * name[array_index]/type/count/matrix_column fields */
>  	const char *first_slash = strchr(text, '/');
>  	if (first_slash == NULL) {
> -		printf("Column headers must be in the form name/type/count/matrix_column.\n"
> -		       "Note: matrix_column is optional.\n"
> +		printf("Column headers must be in the form"
> +		       " name[array_index]/type/count/matrix_column.\n"
> +		       "Note: [array_index] and matrix_column are optional.\n"
>  		       "Got: %s\n",
>  		       text);
>  		piglit_report_result(PIGLIT_FAIL);
>  	}
> +

Is this extra line needed?

>  	std::string name(text, first_slash);
> +
> +	/* If the attrib is an array, strip the index */
> +	if (name[name.size() - 1] == ']') {
> +		std::string::size_type n = name.find('[');
> +		if (n == std::string::npos) {
> +			printf("Column header looked like an array"
> +			       " but couldn't parse it.\n"
> +			       "Got: %s\n",
> +			       text);
> +			piglit_report_result(PIGLIT_FAIL);
> +		} else {
> +			this->array_index = strtoul(name.substr(n + 1).c_str(), NULL, 0);
> +			name.resize(n);
> +		}
> +	} else {
> +		this->array_index = 0;
> +	}
> +
>  	const char *second_slash = strchr(first_slash + 1, '/');
>  	if (second_slash == NULL) {
> -		printf("Column headers must be in the form name/type/count/matrix_column.\n"
> +		printf("Column headers must be in the form"
> +		       " name[array_index]/type/count/matrix_column.\n"
> +		       "Note: [array_index] and matrix_column are optional.\n"
>  		       "Got: %s\n",
>  		       text);
>  		piglit_report_result(PIGLIT_FAIL);
>  	}
>  	std::string type_str(first_slash + 1, second_slash);
> -	this->data_type = decode_type(type_str.c_str());
> +	this->data_type = decode_type(type_str.c_str(),
> +				      &this->count, &this->data_type_cols);
>  	char *endptr;
>  	this->count = strtoul(second_slash + 1, &endptr, 10);
>  
> @@ -217,8 +297,9 @@ vertex_attrib_description::vertex_attrib_description(GLuint prog,
>  		}
>  
>  		if (*endptr != '\0') {
> -			printf("Column headers must be in the form name/type/matrix_column.\n"
> -			       "Note: matrix_column is optional.\n"
> +			printf("Column headers must be in the form"
> +			       " name[array_index]/type/count/matrix_column.\n"
> +			       "Note: [array_index] and matrix_column are optional.\n"
>  			       "Got: %s\n",
>  			       text);
>  			piglit_report_result(PIGLIT_FAIL);
> @@ -323,9 +404,11 @@ void
>  vertex_attrib_description::setup(size_t *offset, size_t stride) const
>  {
>  	int attribute_size = ATTRIBUTE_SIZE;
> +	GLuint actual_index = this->index + this->matrix_index
> +		+ this->array_index * this->data_type_cols;
>  	switch (this->data_type) {
>  	case GL_FLOAT:
> -		glVertexAttribPointer(this->index + this->matrix_index, this->count,
> +		glVertexAttribPointer(actual_index, this->count,
>  				      this->data_type, GL_FALSE, stride,
>  				      (void *) *offset);
>  		break;
> @@ -334,9 +417,9 @@ vertex_attrib_description::setup(size_t *offset, size_t stride) const
>  			fprintf(stderr,"vertex_attrib_description fail. no 64-bit float support\n");
>  			return;
>  		}
> -		glVertexAttribLPointer(this->index + this->matrix_index, this->count,
> -				      this->data_type, stride,
> -				      (void *) *offset);
> +		glVertexAttribLPointer(actual_index, this->count,
> +				       this->data_type, stride,
> +				       (void *) *offset);
>  		attribute_size *= 2;
>  		break;
>  	default:
> @@ -344,12 +427,11 @@ vertex_attrib_description::setup(size_t *offset, size_t stride) const
>  			fprintf(stderr,"vertex_attrib_description fail. no int support\n");
>  			return;
>  		}
> -		glVertexAttribIPointer(this->index + this->matrix_index, this->count,
> +		glVertexAttribIPointer(actual_index, this->count,
>  				       this->data_type, stride,
> -				       (void *) *offset);
> -		break;
> +				       (void *) *offset);		break;
>  	}
> -	glEnableVertexAttribArray(index + this->matrix_index);
> +	glEnableVertexAttribArray(actual_index);
>  	*offset += attribute_size * this->count;
>  }
>
On Mon, 2016-05-02 at 11:07 +0200, Alejandro Piñeiro wrote:
> On 25/04/16 15:56, Andres Gomez wrote:

[snip]

> > In addition to the now recommended format:
> > attname/vec3/3
> Nitpick: Are you sure that is the recommended instead of just the
> extended one?

Well, I want to make it the recommended since the previous format
doesn't allow full use of arrays and is kind of ambiguous against the
extended one.

[snip]

> > + * the data type but that has been kept for backward
> > + * compatibility.
> Now that you mention backward compatibility, I think that it would be
> good to include some examples with the old format. After all, is the
> format of most tests right now.

Makes sense.

[snip]

> > > > @@ -126,13 +147,38 @@ decode_type(const char *type)
> >  		{ "uint",    GL_UNSIGNED_INT   },
> >  		{ "float",   GL_FLOAT          },
> >  		{ "double",  GL_DOUBLE         },
> > +		{ "ivec",    GL_INT            },
> > +		{ "uvec",    GL_UNSIGNED_INT   },
> > +		{ "vec",     GL_FLOAT          },
> > +		{ "dvec",    GL_DOUBLE         },
> > +		{ "mat",     GL_FLOAT          },
> > +		{ "dmat",    GL_DOUBLE         },
> >  		{ NULL,      0                 }
> >  	};
> >  
> >  
> >  	for (int i = 0; type_table[i].type; ++i) {
> > -		if (0 == strcmp(type, type_table[i].type))
> > +		if (0 == strncmp(type, type_table[i].type,
> > strlen(type_table[i].type))) {
> > +			if (i > 3 && (rows || cols)) 
> What 3 is lacks somewhat a context, as you need to go back to
> type_table
> to know what you want to do. Couldn't it be replaced by a kind of
> enum
> or at least a comment?

Makes sense, I will add a comment.

> > 
> > +				size_t type_len = strlen(type);
> > +				size_t the_rows = type[type_len -
> > 1] - '0';
> > +				if (rows)
> > +					*rows = the_rows;
> > +				if (cols) {
> > +					if (i > 7)
> Ditto

Ditto :)

[snip]

> >  	/**
> > +	 * Number of cols in the data type of this attribute.
> For the comment s/cols/columns

Will do.

[snip]

> > @@ -184,25 +240,49 @@ public:
> >  vertex_attrib_description::vertex_attrib_description(GLuint prog,
> >  						     const char
> > *text)
> >  {
> > -	/* Split the column header into name/type/count fields */
> > +	/* Split the column header into
> > +	 * name[array_index]/type/count/matrix_column fields */
> >  	const char *first_slash = strchr(text, '/');
> >  	if (first_slash == NULL) {
> > -		printf("Column headers must be in the form
> > name/type/count/matrix_column.\n"
> > -		       "Note: matrix_column is optional.\n"
> > +		printf("Column headers must be in the form"
> > +		       "
> > name[array_index]/type/count/matrix_column.\n"
> > +		       "Note: [array_index] and matrix_column are
> > optional.\n"
> >  		       "Got: %s\n",
> >  		       text);
> >  		piglit_report_result(PIGLIT_FAIL);
> >  	}
> > +
> Is this extra line needed?

It makes it more readable for me. Previously, we were not treating the
"name" variable at all and now we process it so it made sense to me to
separate the whole block of declaring the "name" variable and
processing it.

> 
> > 
> >  	std::string name(text, first_slash);
> > +
> > +	/* If the attrib is an array, strip the index */
> > +	if (name[name.size() - 1] == ']') {
> > +		std::string::size_type n = name.find('[');
> > +		if (n == std::string::npos) {
> > +			printf("Column header looked like an
> > array"
> > +			       " but couldn't parse it.\n"
> > +			       "Got: %s\n",
> > +			       text);
> > +			piglit_report_result(PIGLIT_FAIL);
> > +		} else {
> > +			this->array_index = strtoul(name.substr(n
> > + 1).c_str(), NULL, 0);
> > +			name.resize(n);
> > +		}
> > +	} else {
> > +		this->array_index = 0;
> > +	}
> > +
> >  	const char *second_slash = strchr(first_slash + 1, '/');

I'll send a new version of the patch. Thanks!