[PATCH] Pragma optimize on/off

[Aaron Ballman]

On Wed, May 7, 2014 at 11:16 AM, Dario Domizioli
<dario.domizioli at gmail.com> wrote:
> Hi Aaron.
> Thank you for the review. I'm attaching a new patch.
>
>
> On 7 May 2014 15:10, Aaron Ballman <aaron at aaronballman.com> 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