[PATCH] Pragma optimize on/off

[Richard Smith]

On Fri, May 16, 2014 at 6:53 AM, Dario Domizioli
<dario.domizioli at gmail.com>wrote:

> Hello again, everybody.
>
> Here is an updated and rebased patch which addresses Sean's comments about
> the very ambiguous diagnostic message. I have now made it an error - after
> all, extra arguments mean that the pragma as written has an invalid syntax
> and the user intent cannot really be guessed.
> I have also added a check that the pragma works with the _Pragma operator
> as well (so that you can #define it).
>
> I think we're converging on a solid patch.
> @Richard: could you tell me if you are satisfied with the serialization
> support as implemented?
>

+  WriteOptimizePragmaOptions(SemaRef);

Please guard this by if (!WritingModule). I don't think it makes sense to
inherit this state from a module (and I definitely don't want to worry
about merging it if multiple modules disagree about the state). Since it
might then be missing...

+  // Update the state of 'pragma clang optimize'. Use the same API as if
we had
+  // encountered the pragma in the source.
+  bool IsOn = !OptimizeOffPragmaLocation.isValid();
+  SemaObj->ActOnPragmaOptimize(IsOn, OptimizeOffPragmaLocation);

... only call ActOnPragmaOptimize if the location is valid.

Otherwise, this LGTM.


> Thanks,
>     Dario Domizioli
>     SN Systems - Sony Computer Entertainment Group
>
>
>
>
>
>
>
>
> On 13 May 2014 20:50, Sean Silva <chisophugis at gmail.com> wrote:
>
>>
>>
>>
>> On Mon, May 12, 2014 at 1:32 PM, Dario Domizioli <
>> dario.domizioli at gmail.com> wrote:
>>
>>> Hi Sean.
>>>
>>>
>>> On 12 May 2014 19:36, Sean Silva <chisophugis at gmail.com> wrote:
>>>
>>>> +#pragma clang optimize on top of spaghetti  // expected-warning
>>>> {{extra tokens at end of '#pragma clang optimize' - ignored}}
>>>> The diagnostic here is ambiguous. Was the pragma itself ignored or were
>>>> the extra tokens ignored?
>>>>
>>>>
>>> Hm... The whole pragma is ignored in that case. Some other pragmas
>>> behave the same - if they are not correctly formatted and they have
>>> additional argument they are ignored as a whole. Now that I think of it, I
>>> agree that this behaviour is confusing.
>>> I have maintained consistency with other pragmas (ahem... ok, maybe I
>>> have copied the check from another pragma handler :-) ), but I could easily
>>> change this specific case to an error. In fact, now I think it would make
>>> more sense.
>>>
>>> As for the actual text being printed out, the problem is that I have
>>> leveraged an existing warning message. I could change the message, but it
>>> would suddenly become different in all the pragmas that make use of the
>>> message. If we do that, it would probably be better to make it a separate,
>>> documented change.
>>>
>>
>> Yeah, that definitely would be better suited to being its own change.
>>
>> -- Sean Silva
>>
>>
>>>
>>> Cheers,
>>>     Dario Domizioli
>>>     SN Systems - Sony Computer Entertainment Group
>>>
>>>
>>>
>>>> PS: I actually laughed out loud when I read "on top of spaghetti" :)
>>>>
>>>
>>> I cannot claim the paternity of the phrase since I copied it from
>>> another pragma test. :-) But I found it funny too.
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>>
>>>>  On Thu, May 8, 2014 at 10:43 AM, Dario Domizioli <
>>>> dario.domizioli at gmail.com> wrote:
>>>>
>>>>> Hello again!
>>>>>
>>>>> I have now extended the patch with support for serialization of the
>>>>> state of the pragma (for PCHs). I have followed a similar pattern to the
>>>>> one used for the floating point pragmas and the diagnostic pragmas.
>>>>> I have added a PCH test that verifies that if a "#pragma clang
>>>>> optimize off" is still active at the end of a PCH then a source file
>>>>> compiled including the PCH will behave as if the pragma was specified in
>>>>> the source (as it happens with normal headers). The test is modeled after
>>>>> the pragma diagnostics test.
>>>>>
>>>>> I hope the new attached patch covers the issue that was raised by
>>>>> Richard... but I welcome any feedback. I might have missed something.
>>>>>
>>>>> Cheers,
>>>>>     Dario Domizioli
>>>>>     SN Systems - Sony Computer Entertainment Group
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On 7 May 2014 18:41, Dario Domizioli <dario.domizioli at gmail.com>wrote:
>>>>>
>>>>>>
>>>>>> Thanks Aaron!
>>>>>>
>>>>>> Patch LGTM, modulo Richard's comments about serialization (I worried
>>>>>>> about the same thing, and my preference is for the patch to be
>>>>>>> all-inclusive when feasible, but Richard is welcome to weigh in). I
>>>>>>> have no strong opinions on whether it should be one diagnostic or
>>>>>>> two.
>>>>>>>
>>>>>>
>>>>>> I will wait for Richard's comments then; meanwhile I'll keep working
>>>>>> on the serialization issue.
>>>>>>
>>>>>> As for the diagnostics, I think that having two might make it easier
>>>>>> to extend the "unexpected" case if the pragma gains new functionality in
>>>>>> the future, while the "missing" case is unlikely to change... although the
>>>>>> %select is quite powerful so I haven't got a strong preference either.
>>>>>>
>>>>>> Cheers,
>>>>>>     Dario Domizioli
>>>>>>     SN Systems - Sony Computer Entertainment Group
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140522/0b04255c/attachment.html>