Fwd: patch: remove the unqualified name cache for typos
Kaelyn Takata
rikka at google.com
Mon Jan 5 14:47:27 PST 2015
Just realized I accidently hit "Reply" instead of "Reply All" when sending
this email several weeks ago...
---------- Forwarded message ----------
From: Kaelyn Takata <rikka at google.com>
Date: Sat, Dec 20, 2014 at 1:01 PM
Subject: Re: patch: remove the unqualified name cache for typos
To: Richard Smith <richard at metafoo.co.uk>
On Fri, Dec 19, 2014 at 5:00 PM, Richard Smith <richard at metafoo.co.uk>
wrote:
>
> On Fri, Dec 19, 2014 at 4:46 PM, Kaelyn Takata <rikka at google.com> 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.
> That said, the test changes look like a real improvement.
>
> On Thu, Dec 18, 2014 at 5:34 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>>>
>>> On Tue, Dec 16, 2014 at 9:47 AM, Kaelyn Takata <rikka at google.com> wrote:
>>>>
>>>>
>>>>
>>>> On Tue, Dec 16, 2014 at 12:13 AM, Nick Lewycky <nlewycky at google.com>
>>>> wrote:
>>>>
>>>>> On 15 December 2014 at 18:12, Kaelyn Takata <rikka at google.com> 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 <nlewycky at google.com>
>>>>>> 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
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150105/c37e51c2/attachment.html>
More information about the cfe-commits
mailing list