[PATCH] D91460: [AsmParser] make .ascii support spaces as separators

Fri Dec 4 15:09:35 PST 2020

jrtc27 added inline comments.

================
Comment at: llvm/lib/MC/MCParser/AsmParser.cpp:3005
 /// parseDirectiveAscii:
-///   ::= ( .ascii | .asciz | .string ) [ "string" ( , "string" )* ]
+///   ::= .ascii [ "string" ( ( , ) "string" )* ]
+///   ::= ( .asciz | .string ) [ "string" ( , "string" )* ]
----------------
jcai19 wrote:
> jrtc27 wrote:
> > Hm, this isn't particularly clear as it stands that multiple strings next to each other is valid, it just looks like the comma has been put it in brackets. Maybe this to better reflect how it's parsed (square brackets around the comma instead would also work, but doesn't capture the logical parse tree so well)? I don't know if the `"string"` needs to be put in parentheses though to apply `+` to it; is this a real BNF-esque grammar or just a hand-wavey one for human consumption?
> It appears to be only for human consumption, at least I could not find anywhere it was parsed. For .ascii we could use either space or comma to achieve the same effect, which made comma optional. Based on the original text, I assume brackets mean the enclosed symbol is mandatory while parentheses means it is optional. 
No, parens are for grouping (so `*` applies to multiple items), and square brackets indicate the items are optional. https://en.wikipedia.org/wiki/Backus–Naur_form#Variants describes some of these common BNF-based grammars. In this case I think leaving the parens out of my suggestion is fine, nobody in their right mind would think the `+` means that it applies to the double quote (i.e. that, say, `"string""` is valid), and strictly speaking it should really be `'"' string '"'` or similar if you want it to be a proper formal grammar. So unless you have objections I think my suggestion is the right thing to go with :)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91460