On Fri, May 16, 2014 at 6:53 AM, Dario Domizioli <[email protected]>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 <[email protected]> wrote: > >> >> >> >> On Mon, May 12, 2014 at 1:32 PM, Dario Domizioli < >> [email protected]> wrote: >> >>> Hi Sean. >>> >>> >>> On 12 May 2014 19:36, Sean Silva <[email protected]> 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 < >>>> [email protected]> 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 <[email protected]>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 >>>>> [email protected] >>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>> >>>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
