Address PR comments about parsing, style

Author: stevenfontanella

src/parser/parse-1-decls.cpp

@@ -18,6 +18,8 @@
 
 namespace wasm::WATParser {
 
-Result<> parseDecls(ParseDeclsCtx& decls) { return module(decls); }
+Result<> parseModule(ParseDeclsCtx& decls) { return module(decls); }
+
+Result<> parseModuleBody(ParseDeclsCtx& decls) { return moduleBody(decls); }
 
 } // namespace wasm::WATParser

src/parser/parsers.h

@@ -3881,6 +3881,15 @@ template<typename Ctx> MaybeResult<> modulefield(Ctx& ctx) {
   return ctx.in.err("unrecognized module field");
 }
 
+// (m:modulefield)*
+template<typename Ctx> Result<> moduleBody(Ctx& ctx) {
+  while (auto field = modulefield(ctx)) {
+    CHECK_ERR(field);
+  }
+
+  return Ok{};
+}
+
 // module ::= '(' 'module' id? (m:modulefield)* ')'
 //          | (m:modulefield)* eof
 template<typename Ctx> Result<> module(Ctx& ctx) {
@@ -3892,9 +3901,7 @@ template<typename Ctx> Result<> module(Ctx& ctx) {
     }
   }
 
-  while (auto field = modulefield(ctx)) {
-    CHECK_ERR(field);
-  }
+  CHECK_ERR(moduleBody(ctx));
 
   if (outer && !ctx.in.takeRParen()) {
     return ctx.in.err("expected end of module");

src/parser/wast-parser.cpp

@@ -485,11 +485,11 @@ MaybeResult<ModuleInstantiation> instantiation(Lexer& in) {
   }
 
   auto instanceId = in.takeID();
-  if (!instanceId.has_value()) {
+  if (!instanceId) {
     return in.err("expected an instance id in module instantiation");
   }
   auto moduleId = in.takeID();
-  if (!moduleId.has_value()) {
+  if (!moduleId) {
     return in.err("expected a module id in module instantiation");
   }
 

src/parser/wat-parser-internal.h

@@ -22,7 +22,9 @@
 
 namespace wasm::WATParser {
 
-Result<> parseDecls(ParseDeclsCtx& decls);
+Result<> parseModule(ParseDeclsCtx& decls);
+
+Result<> parseModuleBody(ParseDeclsCtx& decls);
 
 Result<> parseTypeDefs(
   ParseDeclsCtx& decls,

src/parser/wat-parser.cpp

@@ -83,37 +83,58 @@ void propagateDebugLocations(Module& wasm) {
   runner.run();
 }
 
-// Parse module-level declarations.
+// After doing the initial pass, parse types, imports, et.c
+Result<> parseModuleWithDecls(ParseDeclsCtx& decls) {
+  auto typeIndices = createIndexMap(decls.in, decls.typeDefs);
+  CHECK_ERR(typeIndices);
+
+  std::vector<HeapType> types;
+  std::unordered_map<HeapType, std::unordered_map<Name, Index>> typeNames;
+  CHECK_ERR(parseTypeDefs(decls, decls.in, *typeIndices, types, typeNames));
 
-// Parse type definitions.
+  // Parse implicit type definitions and map typeuses without explicit types to
+  // the correct types.
+  std::unordered_map<Index, HeapType> implicitTypes;
+  CHECK_ERR(
+    parseImplicitTypeDefs(decls, decls.in, *typeIndices, types, implicitTypes));
 
-// Parse implicit type definitions and map typeuses without explicit types to
-// the correct types.
+  CHECK_ERR(
+    parseModuleTypes(decls, decls.in, *typeIndices, types, implicitTypes));
+
+  CHECK_ERR(parseDefinitions(
+    decls, decls.in, *typeIndices, types, implicitTypes, typeNames));
+
+  propagateDebugLocations(decls.wasm);
+
+  return Ok{};
+}
 
 Result<> doParseModule(Module& wasm, Lexer& input, bool allowExtra) {
   ParseDeclsCtx decls(input, wasm);
-  CHECK_ERR(parseDecls(decls));
+  CHECK_ERR(parseModule(decls));
   if (!allowExtra && !decls.in.empty()) {
     return decls.in.err("Unexpected tokens after module");
   }
 
-  auto typeIndices = createIndexMap(decls.in, decls.typeDefs);
-  CHECK_ERR(typeIndices);
-
-  std::vector<HeapType> types;
-  std::unordered_map<HeapType, std::unordered_map<Name, Index>> typeNames;
-  CHECK_ERR(parseTypeDefs(decls, input, *typeIndices, types, typeNames));
+  CHECK_ERR(parseModuleWithDecls(decls));
 
-  std::unordered_map<Index, HeapType> implicitTypes;
-  CHECK_ERR(
-    parseImplicitTypeDefs(decls, input, *typeIndices, types, implicitTypes));
+  // decls / parseModule made a copy of `input`. Advance `input` past the parsed
+  // module.
+  input = decls.in;
+  return Ok{};
+}
 
-  CHECK_ERR(parseModuleTypes(decls, input, *typeIndices, types, implicitTypes));
+Result<> doParseModuleBody(Module& wasm, Lexer& input, bool allowExtra) {
+  ParseDeclsCtx decls(input, wasm);
+  CHECK_ERR(parseModuleBody(decls));
+  if (!allowExtra && !decls.in.empty()) {
+    return decls.in.err("Unexpected tokens after module");
+  }
 
-  CHECK_ERR(parseDefinitions(
-    decls, input, *typeIndices, types, implicitTypes, typeNames));
+  CHECK_ERR(parseModuleWithDecls(decls));
 
-  propagateDebugLocations(wasm);
+  // decls / parseModuleBody made a copy of `input`. Advance `input` past the
+  // parsed module.
   input = decls.in;
 
   return Ok{};
@@ -125,20 +146,20 @@ Result<> parseModule(Module& wasm,
                      std::string_view in,
                      std::optional<std::string> filename) {
   Lexer lexer(in, filename);
-  return doParseModule(wasm, lexer, false);
+  return doParseModule(wasm, lexer, /*allowExtra=*/false);
 }
 
 Result<> parseModule(Module& wasm, std::string_view in) {
   Lexer lexer(in);
-  return doParseModule(wasm, lexer, false);
+  return doParseModule(wasm, lexer, /*allowExtra=*/false);
 }
 
 Result<> parseModule(Module& wasm, Lexer& lexer) {
-  return doParseModule(wasm, lexer, true);
+  return doParseModule(wasm, lexer, /*allowExtra=*/true);
 }
 
 Result<> parseModuleBody(Module& wasm, Lexer& lexer) {
-  return doParseModule(wasm, lexer, true);
+  return doParseModuleBody(wasm, lexer, /*allowExtra=*/true);
 }
 
 } // namespace wasm::WATParser

src/tools/wasm-shell.cpp

@@ -137,11 +137,7 @@ struct Shell {
     return Ok{};
   }
 
-  using InstanceInfo = std::pair<std::shared_ptr<ShellExternalInterface>,
-                                 std::shared_ptr<ModuleRunner>>;
-
   Result<> instantiate(Module& wasm, Name instanceName) {
-
     std::shared_ptr<ShellExternalInterface> interface;
     std::shared_ptr<ModuleRunner> instance;
     try {
@@ -179,8 +175,7 @@ struct Shell {
   }
 
   Result<> addRegistration(Register& reg) {
-    wasm::Name instanceName =
-      reg.instanceName.has_value() ? *reg.instanceName : lastModule;
+    Name instanceName = reg.instanceName ? *reg.instanceName : lastModule;
 
     auto instance = instances[instanceName];
     if (!instance) {