On May 3, 2013, at 3:31 PM, David Blaikie <[email protected]> wrote:
> On Mon, Mar 25, 2013 at 3:01 PM, Argyrios Kyrtzidis <[email protected]> wrote: >> On Mar 25, 2013, at 9:21 AM, David Blaikie <[email protected]> wrote: >> >>> On Mon, Mar 25, 2013 at 8:56 AM, Argyrios Kyrtzidis <[email protected]> >>> wrote: >>>> On Mar 21, 2013, at 2:49 PM, David Blaikie <[email protected]> wrote: >>>> >>>>> On Wed, Mar 20, 2013 at 5:57 PM, Argyrios Kyrtzidis <[email protected]> >>>>> wrote: >>>>>> On Mar 20, 2013, at 2:10 PM, David Blaikie <[email protected]> wrote: >>>>>> >>>>>>> On Sat, Mar 16, 2013 at 5:17 PM, Argyrios Kyrtzidis <[email protected]> >>>>>>> wrote: >>>>>>>> On Mar 16, 2013, at 3:54 PM, Richard Smith <[email protected]> >>>>>>>> wrote: >>>>>>>> >>>>>>>> On Sat, Mar 16, 2013 at 10:32 AM, Argyrios Kyrtzidis >>>>>>>> <[email protected]> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> On Mar 16, 2013, at 10:18 AM, David Blaikie <[email protected]> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> On Sat, Mar 16, 2013 at 10:13 AM, Dmitri Gribenko >>>>>>>>>> <[email protected]> >>>>>>>>>> wrote: >>>>>>>>>>> On Sat, Mar 16, 2013 at 3:40 AM, Argyrios Kyrtzidis >>>>>>>>>>> <[email protected]> >>>>>>>>>>> wrote: >>>>>>>>>>>> Author: akirtzidis >>>>>>>>>>>> Date: Fri Mar 15 20:40:35 2013 >>>>>>>>>>>> New Revision: 177218 >>>>>>>>>>>> >>>>>>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=177218&view=rev >>>>>>>>>>>> Log: >>>>>>>>>>>> Remove -Wspellcheck and replace it with a diagnostic option. >>>>>>>>>>>> >>>>>>>>>>>> Thanks to Richard S. for pointing out that the warning would show >>>>>>>>>>>> up >>>>>>>>>>>> with -Weverything. >>>>>>>>>>> >>>>>>>>>>> If we are going to start testing clang this way, it would be better >>>>>>>>>>> to >>>>>>>>>>> design this first, so that adding new 'testing' diagnostics is easy >>>>>>>>>>> *and* does not slow down the normal compilation. I think the second >>>>>>>>>>> part is addressed already. >>>>>>>>>>> >>>>>>>>>>> For example, adding a command line option every time is excessive. >>>>>>>>>>> This option could be renamed to -fclang-debugging-diagnostics, and >>>>>>>>>>> all >>>>>>>>>>> such diagnostics could be placed under a special flag >>>>>>>>>>> -Wclang-debugging. >>>>>>>>>> >>>>>>>>>> I still don't understand the need for this at all. At a glance it >>>>>>>>>> seems like we're adding a positive diagnostic so we can check for the >>>>>>>>>> absence of a diagnostic - but we've never had a need to do this in >>>>>>>>>> the >>>>>>>>>> past. "-verify" fails if a diagnostic is emitted where it isn't >>>>>>>>>> expected so the absence of expected-blah lines is sufficient to test >>>>>>>>>> that we don't emit a diagnostic. >>>>>>>>>> >>>>>>>>>> Am I missing something here? Why are we doing this? >>>>>>>>> >>>>>>>>> This code snippet of an objc method >>>>>>>>> >>>>>>>>> -(void)objc_method: { >>>>>>>>> super.x = 0; >>>>>>>>> } >>>>>>>>> >>>>>>>>> would trigger typo-correction for 'super' silently, without emitting >>>>>>>>> any >>>>>>>>> diagnostic. >>>>>>>>> For the regression test I added I put: >>>>>>>>> >>>>>>>>> typedef int super1; >>>>>>>>> >>>>>>>>> so typo-correction "succeeds" in correcting 'super' to 'super1' and >>>>>>>>> errors >>>>>>>>> are emitted. >>>>>>>>> For regression testing purposes this would be sufficient though I >>>>>>>>> don't >>>>>>>>> like that we would be inferring that a typo-correction did not trigger >>>>>>>>> indirectly (it is possible, though unlikely, that typo-correction >>>>>>>>> would >>>>>>>>> trigger without resolving to the intended identifier) >>>>>>>> >>>>>>>> >>>>>>>> The way we usually handle this is with both a positive and a negative >>>>>>>> test: >>>>>>>> >>>>>>>> struct X { int x; } super1; >>>>>>>> -(void)objc_method: { >>>>>>>> super.x = 0; // expected-no-error >>>>>>>> } >>>>>>>> void c_function() { >>>>>>>> super.x = 0; // expected-error {{did you mean 'super1'}} >>>>>>>> } >>>>>>>> >>>>>>>>> >>>>>>>>> Beyond regression testing I'd like to have a way to get notified when >>>>>>>>> typo-correction is silently triggered for general testing. >>>>>>>> >>>>>>>> >>>>>>>> I'm not convinced that this has sufficient value to justify adding a >>>>>>>> -cc1 >>>>>>>> option for it. Can you elaborate on why this is important? >>>>>>>> >>>>>>>> >>>>>>>> I'm not sure what else I could say beyond repeating myself; >>>>>>>> typo-correction >>>>>>>> is expensive, triggering it needlessly is unacceptable, a -cc1 option >>>>>>>> allows >>>>>>>> making sure that it is not triggered across full project sources. >>>>>>>> I don't see much complexity or maintenance involved for justifying not >>>>>>>> having it. >>>>>>> >>>>>>> Part of the issue here is a somewhat philosophical one - this seems >>>>>>> like a bit of a significant change in the way Clang is tested & >>>>>>> written thus far. To make that step I'd like to make sure it's >>>>>>> appropriately considered (yes, this is somewhat a case of a "slippery >>>>>>> slope" fallacy - if the first step isn't a problem even if the trend >>>>>>> could be, we should object to the trend when it happens, not the first >>>>>>> step itself) >>>>>>> >>>>>>> Admittedly I'm still not entirely understanding the issue here - you >>>>>>> mentioned that we were doing typo correction for cases where we >>>>>>> weren't emitting diagnostics. Which part of your change >>>>>>> demonstrates/fixes that? >>>>>> >>>>>> AFAIK, the way parsing works for 'super' is Sema::ClassifyName is called >>>>>> on it, and if lookup fails to find something, the parser later on >>>>>> handles 'super' itself. >>>>>> Because lookup failed in Sema::ClassifyName typo-correction would kick >>>>>> in. >>>>>> >>>>>>> Your fix seems to be inside the CorrectTypo >>>>>>> function. Shouldn't there be a change to a caller so we don't call >>>>>>> this in some non-diagnostic-emitting case if that's happening? >>>>>> >>>>>> If you think the check is more appropriate to be in ClassifyName, that >>>>>> is fine by me. >>>>> >>>>> Looking at the code a bit (enough to get a bit of a better sense) I'm >>>>> not sure that's the right place either. >>>>> >>>>> Here's the issue: we have a codepath that does typo correction (& >>>>> emits diagnostics if it sees them, etc) & the immediate alternative >>>>> codepath doesn't produce a diagnostic. That's problematically >>>>> asymmetric & could lead to similar bugs in other call sites that don't >>>>> handle the non-corrected case in error later. It seems to me that's a >>>>> fundamentally bad API design & we should fix it. (by always emitting a >>>>> diagnostic down there in ClassifyName - either with a typo correction, >>>>> or without) >>>>> >>>>> Secondly: why aren't we able to classify "super"? Shouldn't we know >>>>> that's a keyword & resolving it as such in ClassifyName? >>>>> >>>>> I think we should probably fix both these issues - I haven't looked >>>>> enough to know how to fix them or I'd have done it, but that's my >>>>> feeling at this point. >>>>> >>>>>>> We don't generally add flags like this for other performance problems >>>>>>> - we have many diagnostics that conditionally test whether their flag >>>>>>> is enabled before doing expensive work to test but don't have compiler >>>>>>> flags to test whether or not this code is executed in non-diagnostic >>>>>>> cases, for example. >>>>>> >>>>>> Do you believe it is worth having a way to automatically check whether >>>>>> typo correction has been triggered at all on a "clean" project (no typos >>>>>> to correct, so typo-correction will just affect performance). >>>>> >>>>> Not necessarily, no. This seems like a perf issue as any other that we >>>>> have bots/profiles/etc for. >>>>> >>>>>> As I pointed out before, typo-correction could silently take up 25-30% >>>>>> of -fsyntax-time on ObjC projects. My viewpoint is that this is >>>>>> expensive enough that it is worth adding a flag to use for making sure >>>>>> we don't regress and catch it if typo-correction is triggered needlessly >>>>>> again. >>>>>> >>>>>> If you agree on the "expensive enough" part but disagree on the >>>>>> methodology, could you recommend some other way ? >>>>> >>>>> Something I probably wouldn't really object to would be a boolean flag >>>>> (not a command line argument or anything, just a boolean variable) in >>>>> Sema (possibly in Debug builds only). Raise the flag during typo >>>>> correction. Assert that the flag is not raised at the end of >>>>> compilation if we never emitted any warnings or errors. >>>> >>>> This seems like a good idea to me, but why "in Debug builds only" ? >>> >>> Sorry, I misspoke - I meant assert builds. I tend to conflate the two >>> unnecessarily/incorrectly. (just that the assert will only be compiled >>> in in an assert build, so there's no need to have the flag & logic to >>> raise the flag being compiled in a non-assert build if it'll never be >>> used, but it's probably not a terribly 'hot' thing so the #ifdefing >>> around the variable declaration & flag raising is probably a minor >>> issue - might be worth doing to avoid any current or future "unused >>> member" warnings in non-assert builds, though) >> >> I don't think the #ifdef noise is worth it currently. If a warning comes up >> in the future then we can evaluate if we should put #ifdefs or just an >> attribute. >> If there are no further objections I'll probably implement your idea early >> next week. >> >> Thanks for reviewing! > > I don't remember seeing this go in - could you update this thread with > the revision number if it has/when it does? Reverted in r181073. When I have some time I may revisit.. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
