On Mon, Jan 5, 2015 at 2:47 PM, Kaelyn Takata <[email protected]> wrote:
> Just realized I accidently hit "Reply" instead of "Reply All" when sending > this email several weeks ago... > > ---------- Forwarded message ---------- > From: Kaelyn Takata <[email protected]> > Date: Sat, Dec 20, 2014 at 1:01 PM > Subject: Re: patch: remove the unqualified name cache for typos > To: Richard Smith <[email protected]> > > > > > On Fri, Dec 19, 2014 at 5:00 PM, Richard Smith <[email protected]> > wrote: >> >> On Fri, Dec 19, 2014 at 4:46 PM, Kaelyn Takata <[email protected]> wrote: >>> >>> Here's a patch I whipped up that changes NamedDecl::getUnderlyingDecl to >>> look back through friend decls to find a non-friend decl, which causes >>> notes to be given in the right place in the test Nick changed >>> (test/CXX/class.access/class.friend/p11.cpp) and in another test with >>> FIXMEs about a couple of notes being in the wrong place >>> (test/SemaTemplate/friend-template.cpp). Doing so seemed to be appropriate >>> behavior for getUnderlyingDecl, instead of adding the code to look back >>> through friend decls to both LookupResult::getFoundDecl and >>> TypoCorrection::addCorrectionDecl. This seem good? >>> >> >> In your scan, you should also skip names in the LocalExtern namespace. In >> particular, given: >> >> void f(int n); >> void g() { void f(int n = 0); } >> struct S { friend void f(int n); }; >> void h() { f(); } >> >> I think you'll skip backwards from line 3 to line 2, but we want to find >> the declaration on line 1. The end result will presumably be that we accept >> this ill-formed code. >> >> I'm also not sure that getUnderlyingDecl is the right place to skip >> friends; my inclination is that this is something that should happen >> internally within name lookup (name lookup should act as if it simply does >> not find these friend declarations), rather than being something done by >> consumers of the lookup result. >> > > I tried adding the logic to the various places where the Lookup* > functions/methods add decls to the LookupResult, and the primary place that > needs it to make the two tests my patch changed work properly breaks > another test in a way that I'm pretty sure is a real failure. The change in > question is: > > diff --git a/lib/Sema/SemaLookup.cpp b/lib/Sema/SemaLookup.cpp > index 3445264..9d85555 100644 > --- a/lib/Sema/SemaLookup.cpp > +++ b/lib/Sema/SemaLookup.cpp > @@ -660,6 +660,13 @@ static void > DeclareImplicitMemberFunctionsWithName(Sema &S, > } > } > > +static NamedDecl* getRealDecl(NamedDecl *ND) { > + Decl *Primary = ND; > + while (Primary && > Primary->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend)) > + Primary = Primary->getPreviousDecl(); > + return Primary ? cast<NamedDecl>(Primary) : ND; > +} > + > // Adds all qualifying matches for a name within a decl context to the > // given lookup result. Returns true if any matches were found. > static bool LookupDirect(Sema &S, LookupResult &R, const DeclContext *DC) > { > @@ -675,7 +682,7 @@ static bool LookupDirect(Sema &S, LookupResult &R, > const DeclContext *DC) { > ++I) { > NamedDecl *D = *I; > if ((D = R.getAcceptableDecl(D))) { > - R.addDecl(D); > + R.addDecl(getRealDecl(D)); > Found = true; > } > } > > And the test that legitimately starts failing is CXX/drs/dr1xx.cpp: > > error: 'error' diagnostics expected but not seen: > File ~/llvm/tools/clang/test/CXX/drs/dr1xx.cpp Line 391: friend > declaration specifying a default argument must be the only declaration > error: 'error' diagnostics seen but not expected: > File ~/llvm/tools/clang/test/CXX/drs/dr1xx.cpp Line 391: missing default > argument on parameter > error: 'note' diagnostics expected but not seen: > File ~/llvm/tools/clang/test/CXX/drs/dr1xx.cpp Line 385: previous > declaration is here > 3 errors generated. > > So the only two options I can see for getting the notes right is the patch > to getUnderlyingDecl that I sent earlier, or my original approach which was > to add the logic for going back through the friend decls to both > TransformTypos::EmitAllDiagnostics and LookupResult::getFoundDecl. > I think there's one more tweak missing here: we should only skip over friend declarations if we're not performing redeclaration lookup. > That said, the test changes look like a real improvement. >> >> On Thu, Dec 18, 2014 at 5:34 PM, Richard Smith <[email protected]> >>> wrote: >>>> >>>> On Tue, Dec 16, 2014 at 9:47 AM, Kaelyn Takata <[email protected]> >>>> wrote: >>>>> >>>>> >>>>> >>>>> On Tue, Dec 16, 2014 at 12:13 AM, Nick Lewycky <[email protected]> >>>>> wrote: >>>>> >>>>>> On 15 December 2014 at 18:12, Kaelyn Takata <[email protected]> wrote: >>>>>>> >>>>>>> In general this patch looks good. And it is probably time to kill >>>>>>> the cache anyway now that so much of the typo correction bypasses it >>>>>>> anyway >>>>>>> (correction callbacks that implement a meaningful ValidateCandidate, and >>>>>>> the delayed typo correction for the most part). A few things that >>>>>>> should be >>>>>>> cleaned up though: >>>>>>> >>>>>>> 1) The declaration of EmptyTypoCorrection >>>>>>> in Sema::makeTypoCorrectionConsumer is unused, >>>>>>> >>>>>>> 2) The IsUnqualifiedLookup can be dropped from >>>>>>> Sema::FailedCorrection now that it is unused within the function, >>>>>>> >>>>>>> 3) The EmptyTypoCorrection and ValidatingCallback variables >>>>>>> in Sema::CorrectTypo can be dropped now that the third argument to >>>>>>> Sema::FailedCorrection is unused, and >>>>>>> >>>>>>> 4) IsUnqualifiedLookup can be made a local variable >>>>>>> within Sema::makeTypoCorrectionConsumer instead of a out-parameter and >>>>>>> the >>>>>>> corresponding variables in CorrectTypo and CorrectTypoDelayed can be >>>>>>> removed, now that CorrectTypo doesn't need to know whether the lookup is >>>>>>> qualified or not (which was simply passed along to various calls to >>>>>>> FailedCorrection). >>>>>>> >>>>>> >>>>>> Done. >>>>>> >>>>>> >>>>>>> However, as this patch is already fairly large (removing the cache, >>>>>>> adding the new flag, and making use of it to reunify the two main typo >>>>>>> correction test files) I'm fine with 2-4 being done in a separate >>>>>>> cleanup >>>>>>> commit. >>>>>>> >>>>>> >>>>>> I wrote the code to remove the cache first then the flag as an add on >>>>>> afterwards. After hitting send I realized that I could split them apart >>>>>> and >>>>>> land the new flag first. Let me know if you need me to do that. >>>>>> >>>>>> The only concern I have with this patch is actually incidental to it: >>>>>>> in test/CXX/class.access/class.friend/p11.cpp that the notes from the >>>>>>> newly >>>>>>> exposed suggestions are wrong--they are pointing to previous friend >>>>>>> declarations (which had also been typo-corrected) instead of to the >>>>>>> actual >>>>>>> declarations of ::test2::foo and ::test2::bar. I suspect this is a >>>>>>> relatively simple fix, probably a matter using the the >>>>>>> primary/canonical/first delcaration instead of simply the most recent >>>>>>> one >>>>>>> for the NamedDecl descendants that are also Redeclarable (such as >>>>>>> FunctionDecl and RecordDecl), though it is also an important one from a >>>>>>> QoI >>>>>>> perspective as that test demonstrates the "XYZ declared here" pointing >>>>>>> to >>>>>>> incorrect and potentially confusing locations. >>>>>>> >>>>>> >>>>>> Yes! That is certainly a bug, I think it's a bug in name lookup >>>>>> actually. That's a problem for another day. >>>>>> >>>>>> I tried adding getCanonicalDecl(), but it breaks other cases. If the >>>>>> first declaration is a friend decl, then that one is now canonical and we >>>>>> will put the note on the friend decl instead of on the non-friend decl >>>>>> after it. For example: >>>>>> >>>>>> error: 'note' diagnostics expected but not seen: >>>>>> File test/CXX/class.access/class.friend/p1.cpp Line 39: 'S2' >>>>>> declared here >>>>>> File test/CXX/class.access/class.friend/p1.cpp Line 39: 'S2' >>>>>> declared here >>>>>> File test/CXX/class.access/class.friend/p1.cpp Line 40: 'g2' >>>>>> declared here >>>>>> File test/CXX/class.access/class.friend/p1.cpp Line 40: 'g2' >>>>>> declared here >>>>>> error: 'note' diagnostics seen but not expected: >>>>>> File test/CXX/class.access/class.friend/p1.cpp Line 36: 'g2' >>>>>> declared here >>>>>> File test/CXX/class.access/class.friend/p1.cpp Line 35: 'S2' >>>>>> declared here >>>>>> File test/CXX/class.access/class.friend/p1.cpp Line 36: 'g2' >>>>>> declared here >>>>>> File test/CXX/class.access/class.friend/p1.cpp Line 35: 'S2' >>>>>> declared here >>>>>> 8 errors generated. >>>>>> >>>>>> I spent a while looking at the innards of name lookup to see what was >>>>>> wrong, and the issue is that LookupDirect returns anything that was >>>>>> passed >>>>>> in to makeDeclVisibleInContext before looking at other decls. Passing >>>>>> friend functions to makeDeclVisibleIn the outer Context is the right >>>>>> thing >>>>>> to do, it might be an ordering issue inside LookupQualifiedName or maybe >>>>>> we >>>>>> shouldn't stop looking just because we found one, or maybe >>>>>> findAcceptableDecl ought to do some more sorting instead of just >>>>>> filtering >>>>>> based on visibility. >>>>>> >>>>> >>>>> Ah, fair enough. Of course it can never be simple! I wonder if what's >>>>> needed is a way to distinguish between a friend decl and a non-friend >>>>> decl... or if we'd have to go so far as to flag decls created as a result >>>>> of some diagnostic like typo correction so that they can be avoided when >>>>> possible while generating notes for subsequent diagnostics. But like you >>>>> said, it's a problem for another day; just stick a TODO or two >>>>> in test/CXX/class.access/class.friend/p11.cpp as a guidepost and reminder >>>>> that those notes shouldn't be referencing the typo-corrected friend decls. >>>>> >>>> >>>> I think this should be pretty straightforward to fix: when name lookup >>>> finds a declaration whose IDNS contains both Ordinary/Tag and Friend, scan >>>> backwards over the redecl chain until you find one that isn't marked Friend >>>> nor LocalExtern (in MS compat mode, there might not be one, so you need to >>>> return the original one in that case). The only tricky part is picking the >>>> right place to insert this check into name lookup. >>>> >>>> - Kaelyn >>>>> >>>>>> >>>>>> Nick >>>>>> >>>>>> On Sat, Dec 13, 2014 at 2:47 AM, Nick Lewycky <[email protected]> >>>>>>> wrote: >>>>>>> >>>>>>>> This patch removes Sema::UnqualifiedTyposCorrected. >>>>>>>> >>>>>>>> I'm not sure this is the right thing to do. As far as I can tell >>>>>>>> this cache can't be made to be correct (wouldn't we need to invalidate >>>>>>>> it >>>>>>>> every time a new decl is declared?) and I'm not convinced it saves us >>>>>>>> much >>>>>>>> compile time. However, I'm not confident in my understanding of the >>>>>>>> code, >>>>>>>> so please do push back if it is beneficial. >>>>>>>> >>>>>>>> One bug it fixes is a case where the same full-expression has the >>>>>>>> two typos in the same identifier, and we emit the correction at the >>>>>>>> SourceRange for the first typo twice instead of once for each place >>>>>>>> it's >>>>>>>> misspelled. This is because the SourceRange is on the TypoCorrection, >>>>>>>> and >>>>>>>> the cache key is an IdentifierInfo. Adding the SourceRange to the >>>>>>>> cache key >>>>>>>> wouldn't make much sense because we wouldn't ever re-use it. (NB, I >>>>>>>> have >>>>>>>> similar feelings about TypoCorrectionFailures which I am not touching >>>>>>>> in >>>>>>>> this patch. Why would we do typo correction twice at the same >>>>>>>> location?) >>>>>>>> >>>>>>>> Removing it improves typo correction quality and there are changes >>>>>>>> to the tests (see p11.cpp, and the last two tests in typo-correction) >>>>>>>> where >>>>>>>> it fired. Because the cutoff for typos is a function of this cache, I >>>>>>>> move >>>>>>>> that to a flag (-fspell-checking-limit=...) and turn the limit off >>>>>>>> entirely >>>>>>>> to merge typo-correction-pt2.cpp and typo-correction.cpp. Also turn it >>>>>>>> up >>>>>>>> from 20 to 50 to accommodate existing tests. Without caching, we run >>>>>>>> up the >>>>>>>> counter more quickly. >>>>>>>> >>>>>>>> Please review! >>>>>>>> >>>>>>>> Nick >>>>>>>> >>>>>>>> >>>>>>> >>>>> >>>>> _______________________________________________ >>>>> cfe-commits mailing list >>>>> [email protected] >>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>> >>>>> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
