[PATCH] D154101: [MC] Add three-state parseDirective as a replacement for ParseDirective

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 added a comment.

In D154101#4465229 <https://reviews.llvm.org/D154101#4465229>, @dschuff wrote:

> In D154101#4463955 <https://reviews.llvm.org/D154101#4463955>, @barannikov88 wrote:
>
>> It is difficult to write good tests for WebAssembly directives becasue its parseDirective does not track column numbers. It needs larger refactoring, and I don't have time for this. I'll have to leave it to someone else.
>
> Understood, thanks for calling that out.

Sorry, I was wrong about that. Here is an example of the current diagnostics:

  /mnt/d/llvm-project/llvm/test/MC/WebAssembly/directives-invalid.s:13:18: error: Unknown type in .globaltype directive: i42
  .globaltype sym, i42
                   ^
  
  /mnt/d/llvm-project/llvm/test/MC/WebAssembly/directives-invalid.s:16:22: error: Expected identifier, got:
  
  .globaltype sym, i32,
                       ^
  /mnt/d/llvm-project/llvm/test/MC/WebAssembly/directives-invalid.s:16:22: error: Unknown type in .globaltype modifier:
  
  .globaltype sym, i32,
                       ^
  
  /mnt/d/llvm-project/llvm/test/MC/WebAssembly/directives-invalid.s:19:23: error: Unknown type in .globaltype modifier: unmutable
  .globaltype sym, i32, unmutable
                        ^
  
          .globaltype     sym, i32, immutable
  /mnt/d/llvm-project/llvm/test/MC/WebAssembly/directives-invalid.s:22:32: error: Expected EOL, instead got: ,
  .globaltype sym, i32, immutable,
                                 ^

That is, column numbers are printed correctly. Still, there is a room for improvement. If you're going to put this into your backlog, here are a few ideas:

- split parseDirective into several methods, one per directive;
- use parseComma(), parseOptionalToken(), parseEOL() instead of custom expect() and isNext();
- report errors via Error() with SMRange argument instead of "got: something" message;
- it is conventional to start diagnostic messages with a lowercase letter.

I can add a few tests verifying the current dignostic messages as-is if you want me to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154101