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