[PATCH] D55149: [WebAssembly] Enforce assembler emits to streamer in order.

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:155
+  // as this is how we recognize the start of a function.
+  MCSymbol *LastLabel;
+
----------------
`= nullptr` here?


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:396
     } else if (DirectiveID.getString() == ".functype") {
+      // This code has to do send thing to the streamer similar to
+      // WebAssemblyAsmPrinter::EmitFunctionBodyStart.
----------------
This sentence doesn't make sense.  typo?


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:408
+        CurrentState = FunctionStart;
+      }
       auto Signature = make_unique<wasm::WasmSignature>();
----------------
aardappel wrote:
> This code is not super elegant, but kinda has to be this way because we have .functype which both introduces a function and ones which just declare external ones. Now if you misspel a label you'll fail to start a new function. 
> 
> More ideal would be split these, and have a .functype with no name that annotates the start of a function (and the assembler enforces only comes after a label) and a different one, e.g. .funcdecl, that does have its own name and is only used for external types. Not sure if such a refactoring is worth it though, after we just re-did all the tests with .functype.
I like the repeated name in the .functype directive because it seems to match existing assembly directives.

If we want to distinguish between types for internal vs external functions perhaps we would want a new directive such as ".extern_functype" but as you say I'm not sure its worth it. 


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55149/new/

https://reviews.llvm.org/D55149