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! > > - David > >> >>> >>> This will catch more cases - it'll work on a per-TU basis even in >>> projects that are currently emitting warnings. It will catch cases >>> where we use typo correction in warnings (if there are any?) that are >>> disabled in a particular project. >>> >>> It's a little invasive to have to have something in such a broad scope >>> as the whole Sema object, but seems viable. >>> >>> - David >> _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
