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