On Tue, Jan 20, 2015 at 4:39 PM, Kaelyn Takata <[email protected]> wrote: > > > On Tue, Jan 20, 2015 at 4:32 PM, Richard Smith <[email protected]> > wrote: >> >> On Tue, Jan 20, 2015 at 4:27 PM, Kaelyn Takata <[email protected]> wrote: >> > >> > >> > On Tue, Jan 20, 2015 at 2:46 PM, Richard Smith <[email protected]> >> > wrote: >> >> >> >> 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). >> > >> > >> > I don't think that's quite right, at least in this case.... the friend >> > declaration on line 78 has the error "template non-type parameter has a >> > different type 'long' in template redeclaration" so it seems weird to >> > point >> > the note from another error to a redeclaration that has an error in it. >> >> That's not what's happening: the surrounding template is instantiated >> twice. The first time it produces >> >> template<typename U, int V> struct /*...*/ >> >> and the second time it produces >> >> template<typename U, long V> struct /*...*/ >> >> It's the mismatch between these two that we're diagnosing. Note that >> the first of these two does not have any errors. > > > Ah, I see. In that case, I'll defer to you as to whether the FIXMEs are even > valid (if they aren't valid, it's probably worth adding a note in their > place briefly explaining why the notes are in the right place--that the > errors on those decls are from a specific instantiation of the template > decl, not the template decl itself).
I think the FIXMEs are valid, as a reminder that we should diagnose that case more clearly. Confusing-but-technically-correct diagnostics aren't worth much more than incorrect diagnostics :) To improve the user experience, we should teach this diagnostic to find a non-friend declaration to point at, but it should be a localized change rather than a change to name lookup. >> > Same >> > for the friend declaration on line 300 vs the actual declaration on line >> > 297. Though it just occurred to me the solution would be to ignore the >> > friend declarations that had errors... but unfortunately they aren't >> > marked >> > as invalid (changing getRealDecl to only get the previous decl when >> > isInvalidDecl is also true for the current decl didn't change which >> > decls >> > the notes referred to). >> >> >> >> >> >> >> >> >>>> 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
