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