Hi Tim,
Mostly trivial suggestions, and one bug caught :-)
On 8 November 2015 at 22:34, Timothy Arceri <t_arc...@yahoo.com.au> wrote:
> From: Timothy Arceri <timothy.arc...@collabora.com>
>
> This patch replaces the old interger constant qualifiers with either
typo "integer"
> the new ast_layout_expression type if the qualifier requires merging
> or ast_expression if the qualifier can't have mulitple declorations
"multiple declarations"
> or if all but he newest qualifier is simply ignored.
>
> This also remove the location field that was temporarily added to
> ast_type_qualifier to keep track of the parser location.
> ---
> src/glsl/ast.h | 33 +++---
> src/glsl/ast_to_hir.cpp | 253
> +++++++++++++++++++++++++---------------
> src/glsl/ast_type.cpp | 81 +++++--------
> src/glsl/glsl_parser.yy | 25 ++--
> src/glsl/glsl_parser_extras.cpp | 43 ++++---
> 5 files changed, 236 insertions(+), 199 deletions(-)
>
> diff --git a/src/glsl/ast.h b/src/glsl/ast.h
> index ef94cff..4fd049c 100644
> --- a/src/glsl/ast.h
> +++ b/src/glsl/ast.h
> @@ -1156,9 +1150,10 @@ private:
> class ast_cs_input_layout : public ast_node
> {
> public:
> - ast_cs_input_layout(const struct YYLTYPE &locp, const unsigned
> *local_size)
> + ast_cs_input_layout(const struct YYLTYPE &locp,
> + ast_layout_expression **local_size)
> {
> - memcpy(this->local_size, local_size, sizeof(this->local_size));
> + memcpy(this->local_size, *local_size, sizeof(this->local_size));
While we do use this in other places in mesa it's quite a bit of abuse
of C++. This will involve setting new[] (template) overrides which use
ralloc and using them. Obviously not part of this patch, but more of a
FYI.
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -2467,11 +2463,11 @@ validate_explicit_location(const struct
> ast_type_qualifier *qual,
> YYLTYPE *loc)
> {
> bool fail = false;
> + unsigned qual_location;
>
> - if (qual->location < 0) {
> - _mesa_glsl_error(loc, state, "invalid location %d specified",
> - qual->location);
> - return;
> + if (!process_qualifier_constant(state, loc, "location",
> + qual->location, &qual_location, 0)) {
> + return;
Most/all(?) other places have their process() in the caller, rather
than in validate().
> @@ -2620,22 +2616,30 @@ validate_layout_qualifiers(const struct
> ast_type_qualifier *qual,
> * Older specifications don't mandate a behavior; we take
> * this as a clarification and always generate the error.
> */
> - if (qual->index < 0 || qual->index > 1) {
> + unsigned qual_index;
> + if (process_qualifier_constant(state, loc, "index",
> + qual->index, &qual_index, 0) &&
> + qual_index > 1) {
> _mesa_glsl_error(loc, state,
> "explicit index may only be 0 or 1");
> } else {
> var->data.explicit_index = true;
> - var->data.index = qual->index;
> + var->data.index = qual_index;
This looks wrong. I recommend splitting out the process and validate
steps. Yes the latter is trivial yet enough to catch you here.
Namely:
Currently we assign garbage into data.index even if process() fails.
> @@ -2839,7 +2843,12 @@ apply_type_qualifier_to_variable(const struct
> ast_type_qualifier *qual,
>
> if (state->stage == MESA_SHADER_GEOMETRY &&
> qual->flags.q.out && qual->flags.q.stream) {
Not 100% sure but I'm leaning that we checking
has_explicit_attrib_stream(), or flags.q.explicit_stream. Based
(biased:P) on what other places do. Obviously not meant to be part of
this patch.
> - var->data.stream = qual->stream;
> + unsigned qual_stream;
> + if (process_qualifier_constant(state, loc, "stream", qual->stream,
> + &qual_stream, 0)) {
> + validate_stream_qualifier(loc, state, qual_stream);
There are various approaches of using the process/validate combo in this patch.
Imho it's worth adding a comment when we can ignore skip validate,
and/or ignore its return value.
> @@ -3598,15 +3607,16 @@ static void
> handle_tess_ctrl_shader_output_decl(struct _mesa_glsl_parse_state *state,
> YYLTYPE loc, ir_variable *var)
> {
> - int num_vertices = 0;
> + unsigned num_vertices = 0;
>
> if (state->tcs_output_vertices_specified) {
> - num_vertices = state->out_qualifier->vertices;
> - if (num_vertices <= 0) {
> - _mesa_glsl_error(&loc, state, "invalid vertices (%d) specified",
> - num_vertices);
> + if (!state->out_qualifier->vertices->
> + process_qualifier_constant(state, "vertices",
> + &num_vertices, 0)) {
Off by one ? Original code checks for <= 0 while process() will does <
0. Although on second thought the num_vertices == 0 || num_vertices >
MaxFoo is a validation stage, so we might want to keep it below.
> return;
> - } else if ((unsigned) num_vertices > state->Const.MaxPatchVertices) {
> + }
> +
> + if (num_vertices > state->Const.MaxPatchVertices) {
> _mesa_glsl_error(&loc, state, "vertices (%d) exceeds "
> "GL_MAX_PATCH_VERTICES", num_vertices);
> return;
> @@ -3881,9 +3890,19 @@ ast_declarator_list::hir(exec_list *instructions,
> */
> if (decl_type && decl_type->contains_atomic()) {
> if (type->qualifier.flags.q.explicit_binding &&
> - type->qualifier.flags.q.explicit_offset)
> - state->atomic_counter_offsets[type->qualifier.binding] =
> - type->qualifier.offset;
> + type->qualifier.flags.q.explicit_offset) {
> + unsigned qual_binding;
> + if (process_qualifier_constant(state, &loc, "binding",
> + type->qualifier.binding,
> + &qual_binding, 0)) {
> + unsigned qual_offset;
> + if (process_qualifier_constant(state, &loc, "offset",
> + type->qualifier.offset,
> + &qual_offset, 0)) {
Bikeshed: declare both variables and combine the process(binding) &&
process(offset) into a single if ?
> @@ -6011,8 +6040,12 @@ ast_process_structure_or_interface_block(exec_list
> *instructions,
> const struct ast_type_qualifier *const qual =
> & decl_list->type->qualifier;
>
> - if (qual->flags.q.explicit_binding)
> - validate_binding_qualifier(state, &loc, decl_type, qual);
> + unsigned qual_binding;
> + if (qual->flags.q.explicit_binding &&
> + process_qualifier_constant(state, &loc, "binding",
> + qual->binding, &qual_binding, 0))
Bikeshed: might want to keep the following pattern, or not as long as
it's consistent throughout :)
if (flag)
unsigned qual_foo
if (process(foo))
validate(foo)
> + validate_binding_qualifier(state, &loc, decl_type,
> + qual, qual_binding);
>
> if (qual->flags.q.std140 ||
> qual->flags.q.std430 ||
> @@ -6050,12 +6083,17 @@ ast_process_structure_or_interface_block(exec_list
> *instructions,
> * the specified stream must match the stream associated with the
> * containing block."
> */
> - if (qual->flags.q.explicit_stream &&
> - qual->stream != layout->stream) {
> - _mesa_glsl_error(&loc, state, "stream layout qualifier on "
> - "interface block member `%s' does not match "
> - "the interface block (%d vs %d)",
> - fields[i].name, qual->stream, layout->stream);
> + if (qual->flags.q.explicit_stream && block_stream_processed) {
> + unsigned qual_stream;
> + if (process_qualifier_constant(state, &loc, "stream",
> + qual->stream, &qual_stream, 0)) {
> + if (qual_stream != *block_stream) {
> + _mesa_glsl_error(&loc, state, "stream layout qualifier on "
> + "interface block member `%s' does not "
> + "match the interface block (%d vs %d)",
> + fields[i].name, qual_stream,
> *block_stream);
> + }
> + }
Was going to say "hey
ast_layout_expression::process_qualifier_constant() already does that
for you" only to realise that it deals with a linked list, while we
have an array. Shame we cannot reuse it :(
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
