[PATCH] D26152: [ARM][MC] Cleanup ARM Target Assembly Parser

[Nirav Dave via llvm-commits]

niravd added inline comments.


================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:9280
   else if (IDVal == ".thumb_set")
-    return parseDirectiveThumbSet(DirectiveID.getLoc());
-
-  if (!IsMachO && !IsCOFF) {
+    parseDirectiveThumbSet(DirectiveID.getLoc());
+  else if (!IsMachO && !IsCOFF) {
----------------
rengolin wrote:
> Why no return here, too?
parseDirectiveAlign needed to fail over to the generic align directive parser in AsmParser if it's special case was not encountered, but that violates the pattern here necessitating the return. 


================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:9306
+    return true;
+  return false;
 }
----------------
rengolin wrote:
> Why are we ignoring the return of all the parsing functions?
ParseDirective returns false if the Target Parser is relevant and true if we skip it. This is contrary to the return true on error convention of the parser otherwise. As a result a number of directive parse functions violate the convention which is mostly okay because the results are mostly masked, e.g., we parse two statements in a single pass. I've just isolated the non-standard convention to this case function allowing the other parsing functions to be shorter and clearer.

I plan on going through all of the parsers to fix the end-of-line parsing consistency and pushing all of the non-standardness into the various ParseDirective defs as well. Once they I'll do a once time change to make ParseDirective return true on error and just introspect on the Parser state to see if the target specific parser handled the result or not and the special case for parseDirectiveAlign to fall through will be obviated. 


https://reviews.llvm.org/D26152