Skip to content

[AmberScript] Adding struct support#688

Merged
dj2 merged 3 commits intogoogle:masterfrom
dj2:fmt_11_struct
Oct 15, 2019
Merged

[AmberScript] Adding struct support#688
dj2 merged 3 commits intogoogle:masterfrom
dj2:fmt_11_struct

Conversation

@dj2
Copy link
Collaborator

@dj2 dj2 commented Oct 10, 2019

This CL adds support for the STRUCT syntax in AmberScript. STRUCTS can
be used in buffers. They can be compared as expected. If no OFFSET or
STRIDE data is provided we will calculate the padding and offsets based
on the current layout.

Fixes #544

This CL adds support for the STRUCT syntax in AmberScript. STRUCTS can
be used in buffers. They can be compared as expected. If no OFFSET or
STRIDE data is provided we will calculate the padding and offsets based
on the current layout.

Fixes #544
@dj2 dj2 added enhancement New feature or request amberscript Work related to AmberScript labels Oct 10, 2019
@dj2 dj2 requested a review from alan-baker October 10, 2019 19:42
@dj2 dj2 self-assigned this Oct 10, 2019
Copy link
Collaborator

@alan-baker alan-baker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional to allow partial initialization in ParseValues

return Result("invalid value for STRUCT member OFFSET");

m->offset_in_bytes = token->AsInt32();
} else if (token->AsString() == "ARRAY_STRIDE") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this produce an error if the member is not an array?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return Result("invalid value for STRUCT member ARRAY_STRIDE");

m->array_stride_in_bytes = token->AsInt32();
} else if (token->AsString() == "MATRIX_STRIDE") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question for matrices.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


v.SetIntValue(token->AsUint64());
}
++seg_idx;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, vec4 is 4 segments? How are runtime arrays to be handled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A vec4 would be 4 segments, yes. My plan for runtime arrays is that you end up looping over that group of segments again. So, the start of the array will be flagged and you can have 0 or more of them as needed.

NameData{"mat2x2"},
NameData{"mat2x2<>"})); // NOLINT(whitespace/parens)

TEST_F(AmberScriptParserTest, BufferWithStructStd140) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need some tests with vectors, arrays and runtime arrays.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added vec test. Array and runtime array will be followups when we add the array syntax.


v.SetIntValue(token->AsUint64());
}
++seg_idx;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to allow partial initialization because I don't see a check that all the segments are intialized. Just want to confirm it is intentional. I haven't noticed tests for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test. The buffer code will reject a mismatched number of items currently.

@dj2 dj2 merged commit b0708a9 into google:master Oct 15, 2019
@dj2 dj2 deleted the fmt_11_struct branch October 15, 2019 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

amberscript Work related to AmberScript enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AmberScript: Support a buffer with values of multiple types

2 participants