[PATCH] D32792: [LLVM][inline-asm] Altmacro string escape character '!'

[Renato Golin via Phabricator via llvm-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

As parsers go, simplicity and elegance are usually mutually exclusive.

I agree with you that, in this case, simplicity is more important, even if that generates a bit of redundancy, which isn't really that much.

LGTM. Thanks!



================
Comment at: lib/MC/MCParser/AsmParser.cpp:1207
   while ((*CharPtr != '>') && (*CharPtr != '\'') && (*CharPtr != '\n') &&
 	     (*CharPtr != '\r') && (*CharPtr != '\0')){
+	  if(*CharPtr == '!')
----------------
m_zuckerman wrote:
> rengolin wrote:
> > This could be:
> > 
> >     while ( ((...) && (*CharPtr != '\0')) || (*CharPtr == '!')) {
> >       CharPtr++;
> It will not work since you need double promotion. 
> Consider the following code <5!>4> you want to jump into the '4' char. For that, you will need double promotion. 
of course!


================
Comment at: lib/MC/MCParser/AsmParser.cpp:1219
+/// \brief creating a string without the escape characters '!'. 
+void AsmParser::altMacroString(StringRef AltMacroStr,std::string &Res) {
+  for (int Pos = 0; Pos < AltMacroStr.size(); Pos++) {
----------------
m_zuckerman wrote:
> rengolin wrote:
> > This should really use `isAltmacroString` to make sure it really is a string, or it could have the same problems you detect on the other cases. But doing that, the same logic would need to be implemented twice.
> > 
> > A better approach would be if `isAltmacroString` was actually just `altMacroString` and, if the result is not empty, it "is an alt-macro string".
> > 
> > 
> This function called after "isAltMacroString" validates the string D32701.
> How the flow works: 
>   #  One of the argument contains the '<' sign (parseMacroArguments). 
>   #  This argument is then validated by the second "else if" in the "parseMacroArguments" function(D32701) 
>   #  If the string begins with '<' and ends with '>' the function(parseMacroArguments) marks him as a string.
>   #  Only token that begins with '<' and marked as a string can enter to this function. 
>  
> The problem with your approach is that we need to create a new string that not contain the '!'. This we can't do unless we will create a new global string in the begin of the main function "handleMacroEntry". Since the Stringref only contains the reference. We can't modify him and only mark him.
> 
> What you think, Do you prefer to create a new global string in the begin of the function and then use it? 
Dang, I hate parsers! They're always full of chickens and eggs.


https://reviews.llvm.org/D32792