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? _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
