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" ? > > 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
