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

[Argyrios Kyrtzidis]

On Mar 20, 2013, at 2:10 PM, David Blaikie <dblaikie at gmail.com> wrote:

> On Sat, Mar 16, 2013 at 5:17 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>> On Mar 16, 2013, at 3:54 PM, Richard Smith <richard at metafoo.co.uk> 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.
>>> 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.
>> 
>>