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 <dsinclair@chromium.org>
Reviewed-by: David Neto <dneto@google.com>

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<ast::StructMember> 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<ast::StructMember> 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())
+  if (!t.IsAttrLeft()) {
-    return {};
+    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

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<ast::StructMember> 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<ast::StructMemberDecoration> struct_member_decoration();

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);
+  ASSERT_EQ(decos.size(), 1u);
-  EXPECT_TRUE(deco[0]->IsOffset());
+  EXPECT_TRUE(decos[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");
 }
 
 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");
 }

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<ast::type::I32Type>());
+
+  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();

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) {
+        make_indent();
-            out_ << ", ";
-          }
 
-          first = false;
         // TODO(dsinclair): Split this out when we have more then one
         assert(deco->IsOffset());
-
+        out_ << "[[offset(" << deco->AsOffset()->offset() << ")]]" << std::endl;
-          out_ << "offset(" << deco->AsOffset()->offset() << ")";
       }
-        out_ << "]] ";
+      make_indent();
-      }
-
       out_ << mem->name() << " : ";
       if (!EmitType(mem->type())) {
         return false;

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;
 };
 )");
 }

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;
 })");
 }