On Mon, Mar 25, 2013 at 8:56 AM, Argyrios Kyrtzidis <[email protected]> wrote: > On Mar 21, 2013, at 2:49 PM, David Blaikie <[email protected]> wrote: > >> On Wed, Mar 20, 2013 at 5:57 PM, Argyrios Kyrtzidis <[email protected]> >> wrote: >>> On Mar 20, 2013, at 2:10 PM, David Blaikie <[email protected]> wrote: >>> >>>> On Sat, Mar 16, 2013 at 5:17 PM, Argyrios Kyrtzidis <[email protected]> >>>> wrote: >>>>> On Mar 16, 2013, at 3:54 PM, Richard Smith <[email protected]> wrote: >>>>> >>>>> On Sat, Mar 16, 2013 at 10:32 AM, Argyrios Kyrtzidis <[email protected]> >>>>> wrote: >>>>>> >>>>>> On Mar 16, 2013, at 10:18 AM, David Blaikie <[email protected]> wrote: >>>>>> >>>>>>> On Sat, Mar 16, 2013 at 10:13 AM, Dmitri Gribenko <[email protected]> >>>>>>> wrote: >>>>>>>> On Sat, Mar 16, 2013 at 3:40 AM, Argyrios Kyrtzidis <[email protected]> >>>>>>>> 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) - David > >> >> 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 > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
