This is an archive of the discontinued LLVM Phabricator instance.

[mips] Improve the error messages given by MipsAsmParser.
ClosedPublic

Authored by tomatabacu on Aug 26 2014, 8:22 AM.

Details

Summary

Changed error messages to be more informative and to resemble other clang/llvm error messages (first letter is lower case, no ending punctuation) and updated corresponding tests.

Diff Detail

Event Timeline

tomatabacu updated this revision to Diff 12948.Aug 26 2014, 8:22 AM
tomatabacu retitled this revision from to [mips] Improve the error messages given by MipsAsmParser..
tomatabacu updated this object.
tomatabacu edited the test plan for this revision. (Show Details)
tomatabacu added a reviewer: dsanders.
dsanders edited edge metadata.Sep 4 2014, 7:12 AM

Thanks for going through these. There's a couple unrelated changes and a couple errors that I think could be better.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
2485

Could you drop this line and change AtRegNo to unsigned in a follow up?

2509–2513

Similarly, can you drop this FIXME and make the change in a follow up

2579

I'm not sure this one is an improvement. Most error messages don't address the user with things like 'you must ...'.

3049

I'm not sure about this one. I agree that we don't need to repeat the context since it's already printed in the location part of the error, but 'unknown option' seems a rather brief. Perhaps "unknown option, expected 'pic0' or 'pic2'"?

There's a couple unrelated changes ...

Sorry, clicked submit too soon. I mean there's a couple unrelated changes in the patch that should be in a separate patch.

tomatabacu updated this revision to Diff 13266.Sep 4 2014, 7:47 AM
tomatabacu edited edge metadata.

Addressed concerns.

tomatabacu updated this revision to Diff 13267.Sep 4 2014, 7:57 AM

Used the right arc diff command this time.

tomatabacu updated this revision to Diff 13270.Sep 4 2014, 8:26 AM

Updated warning message in '.option WithBadOption' test case.

dsanders accepted this revision.Sep 14 2014, 6:46 AM
dsanders edited edge metadata.

LGTM

This revision is now accepted and ready to land.Sep 14 2014, 6:46 AM
tomatabacu closed this revision.Sep 16 2014, 8:10 AM