On Tue, Jan 20, 2015 at 1:13 PM, Kaelyn Takata <[email protected]> wrote: > > > On Wed, Jan 14, 2015 at 4:54 PM, Richard Smith <[email protected]> > wrote: >> >> 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. > > > I tried my last patch with a slight tweak to only call getRealDecl when not > performing redeclaration lookup, and while it fixes the above error it also > undoes the fix for the note locations. Changing the line with the call to > getRealDecl to "R.addDecl(R.isForRedeclaration() ? D : getRealDecl(D));" > results in: > > error: 'note' diagnostics expected but not seen: > File ~/llvm/tools/clang/test/SemaTemplate/friend-template.cpp Line 72: > previous non-type template parameter with type 'int' is here > File ~/llvm/tools/clang/test/SemaTemplate/friend-template.cpp Line 297: > previous non-type template parameter with type 'int' is here > error: 'note' diagnostics seen but not expected: > File ~/llvm/tools/clang/test/SemaTemplate/friend-template.cpp Line 78: > previous non-type template parameter with type 'int' is here > File ~/llvm/tools/clang/test/SemaTemplate/friend-template.cpp Line 300: > previous non-type template parameter with type 'int' is here > 4 errors generated. > > Which puts the seen diagnostics back to what friend-template.cpp currently > expects and has FIXMEs for. :(
I think that's a separate issue: I think that it's correct for redeclaration lookup to find the declaration on line 78 from the instantiation of X3<int>; that is the most recent preceding declaration of the same entity in the same scope (friend injection results in friends being declared in the innermost enclosing non-class scope). >>>> 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
