On Mar 16, 2013, at 11:29 AM, David Blaikie <[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. > > If it didn't emit any diagnostics then what was the bug you were fixing? Typo-correction was triggered needlessly. As I wrote in my r177126 commit this created 2 issues: > 1) Performance issue, since typo-correction with PCH/modules is rather > expensive. > 2) Correctness issue, since if it managed to "correct" 'super' then bogus > compiler errors would > be emitted, Note that I found out about 1) and consequently inferred the existence of 2). It's kinda amazing that we never hit the correctness issue so far. > >> 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) > > It's not a random chance/likely-ness issue, though. The typo > correction algorithm is known, so I would assume we would test it like > any other. > > I realize, of course, that the typo correction algorithm could get > smarter & thus negative cases like this might become negative for > other reasons (eg: because we did more precise typo correction & > didn't suggest an identifier you chose that was similar but "not > similar enough") & then we could regress the original reason & perhaps > eventually trigger typo correction for some other pair of strings in > the same context. That seems a somewhat narrow case, though. (& you > could, if really necessary, provide some guarantees against that by > testing a positive case of the same string pair correction in a > different context where you expect it to fire) > >> 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 sure why this feature would be particularly necessary for > typo-correction as opposed to any other feature of the > compiler/diagnostic system. Typo-correction is particularly expensive, it took 25-30% of -fsyntax-time in a particular file from Adium project due to deserializations. It is important that it's not triggered needlessly so as to not affect performance. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
