r177218 - Remove -Wspellcheck and replace it with a diagnostic option.

[Argyrios Kyrtzidis]

On Mar 16, 2013, at 11:29 AM, David Blaikie <dblaikie at gmail.com> wrote:

> On Sat, Mar 16, 2013 at 10:32 AM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>> On Mar 16, 2013, at 10:18 AM, David Blaikie <dblaikie at gmail.com> wrote:
>> 
>>> On Sat, Mar 16, 2013 at 10:13 AM, Dmitri Gribenko <gribozavr at gmail.com> wrote:
>>>> On Sat, Mar 16, 2013 at 3:40 AM, Argyrios Kyrtzidis <akyrtzi at gmail.com> 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.