[PATCH] Pragma optimize on/off

[Dario Domizioli]

Hi Richard, and all.

Thank you very much for the review of this patch and for the useful
comments.
I have made the final two changes that Richard requested, and I have
committed revision 209510. My first actual commit! :-)

Cheers,
    Dario Domizioli
    SN Systems - Sony Computer Entertainment Group





On 23 May 2014 01:41, Richard Smith <richard at metafoo.co.uk> wrote:

> 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/20140523/e0b1e290/attachment.html>