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

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked 4 inline comments as done.
aardappel added inline comments.


================
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) {}
----------------
jgravelle-google wrote:
> 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.
There is one call-site (for one instance of `TokOp`) where no token is available, so the best I can do is make all of em use Tok and then still have one at that has 2 SMLoc's. Not sure if that is much cleaner in total.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:291
+    Parser.Lex();
+    return false;
+  }
----------------
jgravelle-google wrote:
> Wait, is truthiness here used to represent failure? That's... confusing.
> Oh, that's overridden from `MCTargetAsmParser::ParseInstruction`, cool.
Yup, just following along with what LLVM does here.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:320
+      if (Lexer.is(AsmToken::Integer)) {
+        if (ParserSingleInteger(false, Operands))
+          return true;
----------------
jgravelle-google wrote:
> ... 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.
I'd just swap the truthyness. Since the rest of the code doesn't use such a macro, there's little point I'm afraid.


https://reviews.llvm.org/D44329