[PATCH] D32701: [LLVM][inline-asm][Altmacor] Altmacro string delimiter '<..>'

[michael zuckerman via Phabricator via llvm-commits]

m_zuckerman added inline comments.


================
Comment at: lib/MC/MCParser/AsmParser.cpp:1201
+/// character.
+bool AsmParser::isAltmacroString(SMLoc &StrLoc, SMLoc &EndLoc) {
+  assert((StrLoc.getPointer() != NULL) &&
----------------
rengolin wrote:
> rengolin wrote:
> > I'd add single quotes at the same time.
> > 
> > This seems like a simple addition, as you can pass the expected "ending char" as an argument, in addition to `\r\n\0`.
> The problem here, IIUC, is that you could start with '<' and end with single-quote (or vice-versa) and this code would accept. Also, if you start with '<', single-quote isn't actually a terminator.
> 
> To make sure this works, you have to test four overall patterns:
>   - <foo,bar>
>   - <foo'bar>
>   - 'foo,bar'
>   - 'foo>bar'
> 
> They all have to produce a single string "foo?bar".
> 
> What I meant was something like:
> 
>     bool AsmParser::isAltmacroString(SMLoc &StrLoc, SMLoc &EndLoc, const char Term) {
>       ...
>       while ((*CharPtr != Term) && (*CharPtr != '\n') && (*CharPtr != '\r') && (*CharPtr != '\0')) {
>         CharPtr++;
>     }
> 
> And then down there, something like:
> 
>     } else if (Lexer.IsaAltMacroMode() && Lexer.is(AsmToken::Less) && Lexer.is(AsmToken::SingleQuote) &&
>                isAltmacroString(StrLoc, EndLoc, Lex()) {
> 
> or something.
>     
> 
1. The code will not accept it because the last character that I check is '>'. So if the code ends with "\ '". The function returns false since it does not equal to '>' as required by the second if.

2. regard the <foo'bar>:  I don't really sure what is the usage of it. 
GAS doesn't really support the "\'" delimiter and the only full support that I found using https://godbolt.org/ was for the "<...>".  

3. I prefer only to support '<..>' as in GCC. And I don't mind to define single quote as a string in altmacro, but it must end with a single quote.



https://reviews.llvm.org/D32701