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. > > 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). 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 ? > >> I'm also surprised we hit the typo-correction codepath at all in this case; >> we shouldn't be getting there unless we've already decided to emit a >> diagnostic. >> >> _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
