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

Mon Mar 25 08:56:44 PDT 2013

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

> On Wed, Mar 20, 2013 at 5:57 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>> 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.
> 
> Looking at the code a bit (enough to get a bit of a better sense) I'm
> not sure that's the right place either.
> 
> Here's the issue: we have a codepath that does typo correction (&
> emits diagnostics if it sees them, etc) & the immediate alternative
> codepath doesn't produce a diagnostic. That's problematically
> asymmetric & could lead to similar bugs in other call sites that don't
> handle the non-corrected case in error later. It seems to me that's a
> fundamentally bad API design & we should fix it. (by always emitting a
> diagnostic down there in ClassifyName - either with a typo correction,
> or without)
> 
> Secondly: why aren't we able to classify "super"? Shouldn't we know
> that's a keyword & resolving it as such in ClassifyName?
> 
> I think we should probably fix both these issues - I haven't looked
> enough to know how to fix them or I'd have done it, but that's my
> feeling at this point.
> 
>>> 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).
> 
> Not necessarily, no. This seems like a perf issue as any other that we
> have bots/profiles/etc for.
> 
>> 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 ?
> 
> Something I probably wouldn't really object to would be a boolean flag
> (not a command line argument or anything, just a boolean variable) in
> Sema (possibly in Debug builds only). Raise the flag during typo
> correction. Assert that the flag is not raised at the end of
> compilation if we never emitted any warnings or errors.

This seems like a good idea to me, but why "in Debug builds only" ?

> 
> This will catch more cases - it'll work on a per-TU basis even in
> projects that are currently emitting warnings. It will catch cases
> where we use typo correction in warnings (if there are any?) that are
> disabled in a particular project.
> 
> It's a little invasive to have to have something in such a broad scope
> as the whole Sema object, but seems viable.
> 
> - David