On Wed, May 7, 2014 at 11:16 AM, Dario Domizioli <[email protected]> wrote: > Hi Aaron. > Thank you for the review. I'm attaching a new patch. > > > On 7 May 2014 15:10, Aaron Ballman <[email protected]> wrote: >> >> >> Style change -- else if should be on the same line as } > > > Fixed. > > >> > --- test/SemaCXX/pragma-optimize-diagnostics.cpp (revision 0) >> > +++ test/SemaCXX/pragma-optimize-diagnostics.cpp (revision 0) >> >> This seems like it should be a parser test more than a sema test. > > > Moved to the Parser directory. Also added a few comments and changed the > diagnostic message (see below). > > >> > +// - #pragma clang optimize on/off >> > +def err_pragma_optimize_malformed : Error< >> > + "malformed argument to '#pragma clang optimize' - expected 'on' or >> > 'off'">; >> >> Should probably be worded to use "unexpected" instead of "malformed." >> Perhaps: "unexpected argument '%0'; expected 'on' or 'off'" or >> something akin to that? > > > Adding a %0 is problematic as sometimes the error is that the argument is > missing or is not an identifier, so there's no text to display in the %0. > Rather than adding another diagnostic just for that specific case, I have > changed the error to "pragma clang optimize is malformed; it requires 'on' > or 'off'", which is very similar to an error message a couple of lines above > (the one about pragma detect_mismatch). > Is that satisfactory?
We don't usually use "malformed" in our diagnostic wordings (though it does happen from time to time). I think "unexpected" is more consistent. As for not having text to display, wouldn't it be possible to simply pass in the text as-lexed (via PP.getSpelling())? > > >> > + /// directive if such directive has not been closed by an "on" yet. >> > If >> >> "such directive" should be "such a directive". > > > Fixed. > > Cheers, > Dario Domizioli > SN Systems - Sony Computer Entertainment Group > > ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
