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

[David Blaikie]

On Mon, Mar 25, 2013 at 3:01 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
> On Mar 25, 2013, at 9:21 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>> On Mon, Mar 25, 2013 at 8:56 AM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>>> 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" ?
>>
>> Sorry, I misspoke - I meant assert builds. I tend to conflate the two
>> unnecessarily/incorrectly. (just that the assert will only be compiled
>> in in an assert build, so there's no need to have the flag & logic to
>> raise the flag being compiled in a non-assert build if it'll never be
>> used, but it's probably not a terribly 'hot' thing so the #ifdefing
>> around the variable declaration & flag raising is probably a minor
>> issue - might be worth doing to avoid any current or future "unused
>> member" warnings in non-assert builds, though)
>
> I don't think the #ifdef noise is worth it currently. If a warning comes up in the future then we can evaluate if we should put #ifdefs or just an attribute.
> If there are no further objections I'll probably implement your idea early next week.
>
> Thanks for reviewing!

I don't remember seeing this go in - could you update this thread with
the revision number if it has/when it does?