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