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