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

[Argyrios Kyrtzidis]

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.

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130316/c0df806a/attachment.html>