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

[David Blaikie]

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