[Piglit] [PATCH] util: Add array_index support to [vertex data]

[Andres Gomez]

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!

-- 
Br,

Andres

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/piglit/attachments/20160502/f48fb1ff/attachment-0001.sig>