[PATCH] D44329: [WebAssembly] Added initial AsmParser implementation.

[Jacob Gravelle via Phabricator via llvm-commits]

jgravelle-google added a comment.

WIP review, read up through `WebAssemblyAsmParser::ParseInstruction`



================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:34
+
+#define DEBUG_TYPE "wasm-asmparser"
+
----------------
Nit: I feel this should be "wasm-asm-parser"? The other targets define this as:


> lib/Target/ARM/AsmParser/ARMAsmParser.cpp:#define DEBUG_TYPE "asm-parser"
> lib/Target/Mips/AsmParser/MipsAsmParser.cpp:#define DEBUG_TYPE "mips-asm-parser"
> lib/Target/AVR/AsmParser/AVRAsmParser.cpp:#define DEBUG_TYPE "avr-asm-parser"
> lib/Target/Hexagon/AsmParser/HexagonAsmParser.cpp:#define DEBUG_TYPE "mcasmparser"



so it probably doesn't really matter.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:82
+
+  WebAssemblyOperand(KindTy K, SMLoc Start, SMLoc End, TokOp T)
+    : Kind(K), StartLoc(Start), EndLoc(End), Tok(T) {}
----------------
The repetition here makes me think something is odd, but I'm not convinced that other designs would be better.

One thing that might reduce the conceptual repetition is changing `SMLoc Start, SMLoc End` to `AsmToken Tok`, and just calling `Tok.getLoc()`/`Tok.getEndLoc()` from here, instead of at each callsite.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:117
+    assert(N == 1 && "Invalid number of operands!");
+    assert(isReg() && "not a register operand!");
+    // This is called from the tablegen matcher (MatchInstructionImpl)
----------------
Nit: "not" -> "Not", for consistent capitalization


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:284
+
+  bool ParserSingleInteger(bool IsNegative, OperandVector &Operands) {
+    auto &Int = Lexer.getTok();
----------------
Typo: should be ParseSingleInteger?


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:291
+    Parser.Lex();
+    return false;
+  }
----------------
Wait, is truthiness here used to represent failure? That's... confusing.
Oh, that's overridden from `MCTargetAsmParser::ParseInstruction`, cool.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:320
+      if (Lexer.is(AsmToken::Integer)) {
+        if (ParserSingleInteger(false, Operands))
+          return true;
----------------
... I feel like this sort of thing might be cleaner with a `#define ERR_IF(x) if(x) { return true; }` macro, but I may be in the minority there.


https://reviews.llvm.org/D44329