[PATCH] D10591: Adding altmacro support in integrated assembler.

[Saleem Abdulrasool]

compnerd added a comment.

This seems extremely under-tested.  I remember looking into this, and holding off on the implementation, since it needed some reworking of the expression evaluation, since there are differences in the different modes.


================
Comment at: lib/MC/MCParser/AsmLexer.cpp:505
@@ +504,3 @@
+/// false- a<b 
+bool AsmLexer::isAtString(const char* currentPtr){
+	while ((*currentPtr != ',') && (*currentPtr != '\n') && (*currentPtr != EOF) && (*currentPtr != '\0'))
----------------
Space before the '{'.

================
Comment at: lib/MC/MCParser/AsmLexer.cpp:559
@@ +558,3 @@
+			return LexToken(AltMacro);
+		}else{
+			int len = 1;
----------------
Space around the 'else'

================
Comment at: lib/MC/MCParser/AsmLexer.cpp:607
@@ +606,3 @@
+	case '\'': return LexSingleQuote(AltMacro);
+	case '"': return  LexStringHandel();//LexQuote();
+	case '0': case '1': case '2': case '3': case '4':
----------------
Why leave LexQuote in a comment?

================
Comment at: lib/MC/MCParser/AsmLexer.cpp:612
@@ +611,3 @@
+
+	case '<':{
+		if (isAtString(CurPtr) && AltMacro)
----------------
Space after the colon

================
Comment at: lib/MC/MCParser/AsmLexer.cpp:622
@@ +621,3 @@
+			StringRef(TokStart, 2));
+		default:return AsmToken(AsmToken::Less, StringRef(TokStart, 1));
+		}
----------------
Space after colon

================
Comment at: lib/MC/MCParser/AsmParser.cpp:180
@@ -178,1 +179,3 @@
+	bool AltMacroPresent = false;
+	int64_t valueForTheAltMacro;
 public:
----------------
Variables start with a CapitalLetter.

================
Comment at: lib/MC/MCParser/AsmParser.cpp:250
@@ -246,3 +249,3 @@
 private:
-
+	bool PRE = false;
   bool parseStatement(ParseStatementInfo &Info,
----------------
Partial Redundancy Elimination?

================
Comment at: lib/MC/MCParser/AsmParser.cpp:421
@@ -417,3 +420,3 @@
   bool parseDirectiveMacrosOnOff(StringRef Directive);
-
-  // ".bundle_align_mode"
+  // AltMacros
+  bool parseDirectiveAltMacrosOnOff(StringRef Directive);
----------------
The comment is unnecessary and misleading.

================
Comment at: lib/MC/MCParser/AsmParser.cpp:799
@@ -793,1 +798,3 @@
     return false;
+	case AsmToken::Percent:{
+		if (AltMacroMode)
----------------
Isn't this equivalent to:

    case AsmToken::Percent:
      return !AltMacroMode;

================
Comment at: lib/MC/MCParser/AsmParser.cpp:1241
@@ -1229,2 +1240,3 @@
     IDVal = ".";
-  } else if (parseIdentifier(IDVal)) {
+	}
+	else if (parseIdentifier(IDVal)) {
----------------
unnecessary whitespace change


http://reviews.llvm.org/D10591