[PATCH] D19929: Fix Mips Parser error reporting

[Daniel Sanders via llvm-commits]

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

LGTM with a few small changes.

Could you format this the way clang-format would? In particular there should be a space after the '//' in comments and before the '{' on the if-statements.

Could you also add a test case? Something that checks we don't emit the 'unknown directive' error for one of the incorrect directives is sufficient.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:4884
@@ -4883,2 +4883,3 @@
 
+//FIXME: These two functions should both either eat or not eat
 bool MipsAsmParser::reportParseError(Twine ErrorMsg) {
----------------
If they're going to have the same name then I agree that they should be consistent in behaviour. From their usage it looks like we ought to have both behaviours available so I think it's best to rename one of them. We should leave that for another patch though.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:6039-6041
@@ -6038,2 +6038,5 @@
 bool MipsAsmParser::ParseDirective(AsmToken DirectiveID) {
+  //This returns false if we understand the directive, so even on
+  //errors this should return false.
+
   MCAsmParser &Parser = getParser();
----------------
I disagree with this comment as it's written since most errors are reported because we didn't (fully) understand the directive. However, the caller of ParseDirective() phrases it as "It will return 'true' if it isn't interested in this directive." which I read as "it recognized the first token of the directive and decided to handle it". I think this is what you mean and I agree we don't comply with this at the moment.

I'm thinking it might be better phrased as something like:
> This returns false if this function recognizes the directive regardless of whether it is successfully handled or reports an error. Otherwise it returns true to give the generic parser a chance at recognizing it.


http://reviews.llvm.org/D19929