From 71d69e5ed3f42406b901a5c7babfa38f25a7bdc0 Mon Sep 17 00:00:00 2001 From: dan sinclair Date: Thu, 8 Oct 2020 19:17:45 +0000 Subject: [PATCH] Update struct member decorations to allow multiple blocks. This CL updates the struct member decoration parsing to allow multiple blocks of decorations. Bug: tint:240 Change-Id: I97293ef30333f63c33bbc6e728dba11abc020c7c Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/29280 Commit-Queue: dan sinclair Reviewed-by: David Neto --- src/reader/wgsl/parser_impl.cc | 36 ++++++++++--------- src/reader/wgsl/parser_impl.h | 6 ++-- ...impl_struct_member_decoration_decl_test.cc | 28 +++++++-------- .../wgsl/parser_impl_struct_member_test.cc | 18 ++++++++++ src/writer/wgsl/generator_impl.cc | 22 ++++-------- .../wgsl/generator_impl_alias_type_test.cc | 3 +- src/writer/wgsl/generator_impl_type_test.cc | 6 ++-- 7 files changed, 66 insertions(+), 53 deletions(-) diff --git a/src/reader/wgsl/parser_impl.cc b/src/reader/wgsl/parser_impl.cc index a91712ffc0..032c14cfce 100644 --- a/src/reader/wgsl/parser_impl.cc +++ b/src/reader/wgsl/parser_impl.cc @@ -1557,12 +1557,21 @@ ast::StructMemberList ParserImpl::struct_body_decl() { } // struct_member -// : struct_member_decoration_decl variable_ident_decl SEMICOLON +// : struct_member_decoration_decl+ variable_ident_decl SEMICOLON std::unique_ptr ParserImpl::struct_member() { auto t = peek(); auto source = t.source(); - auto decos = struct_member_decoration_decl(); + ast::StructMemberDecorationList decos; + for (;;) { + size_t s = decos.size(); + if (!struct_member_decoration_decl(decos)) { + return nullptr; + } + if (decos.size() == s) { + break; + } + } if (has_error()) return nullptr; @@ -1590,21 +1599,21 @@ std::unique_ptr ParserImpl::struct_member() { // : // | ATTR_LEFT (struct_member_decoration COMMA)* // struct_member_decoration ATTR_RIGHT -ast::StructMemberDecorationList ParserImpl::struct_member_decoration_decl() { +bool ParserImpl::struct_member_decoration_decl( + ast::StructMemberDecorationList& decos) { auto t = peek(); - if (!t.IsAttrLeft()) - return {}; + if (!t.IsAttrLeft()) { + return true; + } next(); // Consume the peek t = peek(); if (t.IsAttrRight()) { set_error(t, "empty struct member decoration found"); - return {}; + return false; } - ast::StructMemberDecorationList decos; - bool found_offset = false; for (;;) { auto deco = struct_member_decoration(); if (has_error()) @@ -1612,13 +1621,6 @@ ast::StructMemberDecorationList ParserImpl::struct_member_decoration_decl() { if (deco == nullptr) break; - if (deco->IsOffset()) { - if (found_offset) { - set_error(peek(), "duplicate offset decoration found"); - return {}; - } - found_offset = true; - } decos.push_back(std::move(deco)); t = next(); @@ -1628,9 +1630,9 @@ ast::StructMemberDecorationList ParserImpl::struct_member_decoration_decl() { if (!t.IsAttrRight()) { set_error(t, "missing ]] for struct member decoration"); - return {}; + return false; } - return decos; + return true; } // struct_member_decoration diff --git a/src/reader/wgsl/parser_impl.h b/src/reader/wgsl/parser_impl.h index bd7ae82f4c..2d53423ac7 100644 --- a/src/reader/wgsl/parser_impl.h +++ b/src/reader/wgsl/parser_impl.h @@ -170,9 +170,11 @@ class ParserImpl { /// Parses a `struct_member` grammar element /// @returns the struct member or nullptr std::unique_ptr struct_member(); - /// Parses a `struct_member_decoration_decl` grammar element + /// Parses a `struct_member_decoration_decl` grammar element, appending newly + /// parsed decorations to the end of |decos|. + /// @params decos the decoration list /// @returns the list of decorations - ast::StructMemberDecorationList struct_member_decoration_decl(); + bool struct_member_decoration_decl(ast::StructMemberDecorationList& decos); /// Parses a `struct_member_decoration` grammar element /// @returns the decoration or nullptr if none found std::unique_ptr struct_member_decoration(); diff --git a/src/reader/wgsl/parser_impl_struct_member_decoration_decl_test.cc b/src/reader/wgsl/parser_impl_struct_member_decoration_decl_test.cc index 9f57e0eb95..af0f7210ab 100644 --- a/src/reader/wgsl/parser_impl_struct_member_decoration_decl_test.cc +++ b/src/reader/wgsl/parser_impl_struct_member_decoration_decl_test.cc @@ -24,43 +24,41 @@ namespace { TEST_F(ParserImplTest, StructMemberDecorationDecl_EmptyStr) { auto* p = parser(""); - auto deco = p->struct_member_decoration_decl(); + ast::StructMemberDecorationList decos; + ASSERT_TRUE(p->struct_member_decoration_decl(decos)); ASSERT_FALSE(p->has_error()); - EXPECT_EQ(deco.size(), 0u); + EXPECT_EQ(decos.size(), 0u); } TEST_F(ParserImplTest, StructMemberDecorationDecl_EmptyBlock) { auto* p = parser("[[]]"); - auto deco = p->struct_member_decoration_decl(); + ast::StructMemberDecorationList decos; + ASSERT_FALSE(p->struct_member_decoration_decl(decos)); ASSERT_TRUE(p->has_error()); EXPECT_EQ(p->error(), "1:3: empty struct member decoration found"); } TEST_F(ParserImplTest, StructMemberDecorationDecl_Single) { auto* p = parser("[[offset(4)]]"); - auto deco = p->struct_member_decoration_decl(); + ast::StructMemberDecorationList decos; + ASSERT_TRUE(p->struct_member_decoration_decl(decos)); ASSERT_FALSE(p->has_error()); - ASSERT_EQ(deco.size(), 1u); - EXPECT_TRUE(deco[0]->IsOffset()); -} - -TEST_F(ParserImplTest, StructMemberDecorationDecl_HandlesDuplicate) { - auto* p = parser("[[offset(2), offset(4)]]"); - auto deco = p->struct_member_decoration_decl(); - ASSERT_TRUE(p->has_error()) << p->error(); - EXPECT_EQ(p->error(), "1:23: duplicate offset decoration found"); + ASSERT_EQ(decos.size(), 1u); + EXPECT_TRUE(decos[0]->IsOffset()); } TEST_F(ParserImplTest, StructMemberDecorationDecl_InvalidDecoration) { auto* p = parser("[[offset(nan)]]"); - auto deco = p->struct_member_decoration_decl(); + ast::StructMemberDecorationList decos; + ASSERT_FALSE(p->struct_member_decoration_decl(decos)); ASSERT_TRUE(p->has_error()) << p->error(); EXPECT_EQ(p->error(), "1:10: invalid value for offset decoration"); } TEST_F(ParserImplTest, StructMemberDecorationDecl_MissingClose) { auto* p = parser("[[offset(4)"); - auto deco = p->struct_member_decoration_decl(); + ast::StructMemberDecorationList decos; + ASSERT_FALSE(p->struct_member_decoration_decl(decos)); ASSERT_TRUE(p->has_error()) << p->error(); EXPECT_EQ(p->error(), "1:12: missing ]] for struct member decoration"); } diff --git a/src/reader/wgsl/parser_impl_struct_member_test.cc b/src/reader/wgsl/parser_impl_struct_member_test.cc index 7efe135bf3..3c1ca11b0e 100644 --- a/src/reader/wgsl/parser_impl_struct_member_test.cc +++ b/src/reader/wgsl/parser_impl_struct_member_test.cc @@ -52,6 +52,24 @@ TEST_F(ParserImplTest, StructMember_ParsesWithDecoration) { EXPECT_EQ(m->decorations()[0]->AsOffset()->offset(), 2u); } +TEST_F(ParserImplTest, StructMember_ParsesWithMultipleDecorations) { + auto* i32 = tm()->Get(std::make_unique()); + + auto* p = parser(R"([[offset(2)]] +[[offset(4)]] a : i32;)"); + auto m = p->struct_member(); + ASSERT_FALSE(p->has_error()); + ASSERT_NE(m, nullptr); + + EXPECT_EQ(m->name(), "a"); + EXPECT_EQ(m->type(), i32); + EXPECT_EQ(m->decorations().size(), 2u); + EXPECT_TRUE(m->decorations()[0]->IsOffset()); + EXPECT_EQ(m->decorations()[0]->AsOffset()->offset(), 2u); + EXPECT_TRUE(m->decorations()[1]->IsOffset()); + EXPECT_EQ(m->decorations()[1]->AsOffset()->offset(), 4u); +} + TEST_F(ParserImplTest, StructMember_InvalidDecoration) { auto* p = parser("[[offset(nan)]] a : i32;"); auto m = p->struct_member(); diff --git a/src/writer/wgsl/generator_impl.cc b/src/writer/wgsl/generator_impl.cc index 446c92bc9d..187d06ec8e 100644 --- a/src/writer/wgsl/generator_impl.cc +++ b/src/writer/wgsl/generator_impl.cc @@ -444,24 +444,14 @@ bool GeneratorImpl::EmitType(ast::type::Type* type) { increment_indent(); for (const auto& mem : str->members()) { - make_indent(); - if (!mem->decorations().empty()) { - out_ << "[["; - bool first = true; - for (const auto& deco : mem->decorations()) { - if (!first) { - out_ << ", "; - } + for (const auto& deco : mem->decorations()) { + make_indent(); - first = false; - // TODO(dsinclair): Split this out when we have more then one - assert(deco->IsOffset()); - - out_ << "offset(" << deco->AsOffset()->offset() << ")"; - } - out_ << "]] "; + // TODO(dsinclair): Split this out when we have more then one + assert(deco->IsOffset()); + out_ << "[[offset(" << deco->AsOffset()->offset() << ")]]" << std::endl; } - + make_indent(); out_ << mem->name() << " : "; if (!EmitType(mem->type())) { return false; diff --git a/src/writer/wgsl/generator_impl_alias_type_test.cc b/src/writer/wgsl/generator_impl_alias_type_test.cc index e0b04ff9f3..009b2363f9 100644 --- a/src/writer/wgsl/generator_impl_alias_type_test.cc +++ b/src/writer/wgsl/generator_impl_alias_type_test.cc @@ -62,7 +62,8 @@ TEST_F(WgslGeneratorImplTest, EmitAliasType_Struct) { ASSERT_TRUE(g.EmitAliasType(&alias)) << g.error(); EXPECT_EQ(g.result(), R"(type a = struct { a : f32; - [[offset(4)]] b : i32; + [[offset(4)]] + b : i32; }; )"); } diff --git a/src/writer/wgsl/generator_impl_type_test.cc b/src/writer/wgsl/generator_impl_type_test.cc index 495e1e4cd0..d7360b34bc 100644 --- a/src/writer/wgsl/generator_impl_type_test.cc +++ b/src/writer/wgsl/generator_impl_type_test.cc @@ -143,7 +143,8 @@ TEST_F(WgslGeneratorImplTest, EmitType_Struct) { ASSERT_TRUE(g.EmitType(&s)) << g.error(); EXPECT_EQ(g.result(), R"(struct { a : i32; - [[offset(4)]] b : f32; + [[offset(4)]] + b : f32; })"); } @@ -173,7 +174,8 @@ TEST_F(WgslGeneratorImplTest, EmitType_Struct_WithDecoration) { EXPECT_EQ(g.result(), R"([[block]] struct { a : i32; - [[offset(4)]] b : f32; + [[offset(4)]] + b : f32; })"); }