patch: remove the unqualified name cache for typos
Kaelyn Takata
rikka at google.com
Fri Dec 19 16:46:11 PST 2014
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?
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/20141219/1a9c8249/attachment.html>
-------------- next part --------------
diff --git a/include/clang/AST/Decl.h b/include/clang/AST/Decl.h
index cdba3bd..9403545 100644
--- a/include/clang/AST/Decl.h
+++ b/include/clang/AST/Decl.h
@@ -276,7 +276,8 @@ public:
NamedDecl *getUnderlyingDecl() {
// Fast-path the common case.
if (this->getKind() != UsingShadow &&
- this->getKind() != ObjCCompatibleAlias)
+ this->getKind() != ObjCCompatibleAlias &&
+ !this->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend))
return this;
return getUnderlyingDeclImpl();
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
index e43c28a..8a898f1 100644
--- a/lib/AST/Decl.cpp
+++ b/lib/AST/Decl.cpp
@@ -1527,7 +1527,10 @@ NamedDecl *NamedDecl::getUnderlyingDeclImpl() {
if (ObjCCompatibleAliasDecl *AD = dyn_cast<ObjCCompatibleAliasDecl>(ND))
return AD->getClassInterface();
- return ND;
+ Decl *Primary = ND;
+ while (Primary && Primary->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend))
+ Primary = Primary->getPreviousDecl();
+ return Primary ? cast<NamedDecl>(Primary) : ND;
}
bool NamedDecl::isCXXInstanceMember() const {
diff --git a/test/CXX/class.access/class.friend/p11.cpp b/test/CXX/class.access/class.friend/p11.cpp
index a5107fd..15bbda5 100644
--- a/test/CXX/class.access/class.friend/p11.cpp
+++ b/test/CXX/class.access/class.friend/p11.cpp
@@ -19,13 +19,11 @@ namespace test1 {
}
namespace test2 {
- void bar(); // expected-note {{'::test2::bar' declared here}}
+ void bar(); // expected-note 3 {{'::test2::bar' declared here}}
- void foo() { // expected-note {{'::test2::foo' declared here}}
+ void foo() { // expected-note 2 {{'::test2::foo' declared here}}
struct S1 {
friend void foo(); // expected-error {{no matching function 'foo' found in local scope; did you mean '::test2::foo'?}}
- // expected-note at -1{{'::test2::foo' declared here}}
- // TODO: the above note should go on line 24
};
void foo(); // expected-note {{local declaration nearly matches}}
@@ -48,8 +46,6 @@ namespace test2 {
struct S4 {
friend void bar(); // expected-error {{no matching function 'bar' found in local scope; did you mean '::test2::bar'?}}
- // expected-note at -1 2 {{'::test2::bar' declared here}}
- // TODO: the above two notes should go on line 22
};
{ void bar(); }
diff --git a/test/SemaTemplate/friend-template.cpp b/test/SemaTemplate/friend-template.cpp
index e9b2b9b..a3c6709 100644
--- a/test/SemaTemplate/friend-template.cpp
+++ b/test/SemaTemplate/friend-template.cpp
@@ -69,16 +69,13 @@ namespace test3 {
template<typename T, T Value> struct X2a;
- template<typename T, int Size> struct X2b;
+ template<typename T, int Size> struct X2b; // expected-note {{previous non-type template parameter with type 'int' is here}}
template<typename T>
class X3 {
template<typename U, U Value> friend struct X2a;
- // FIXME: the redeclaration note ends up here because redeclaration
- // lookup ends up finding the friend target from X3<int>.
- template<typename U, T Value> friend struct X2b; // expected-error {{template non-type parameter has a different type 'long' in template redeclaration}} \
- // expected-note {{previous non-type template parameter with type 'int' is here}}
+ template<typename U, T Value> friend struct X2b; // expected-error {{template non-type parameter has a different type 'long' in template redeclaration}}
};
X3<int> x3i; // okay
@@ -297,14 +294,11 @@ namespace PR12585 {
int n = C::D<void*>().f();
struct F {
- template<int> struct G;
+ template<int> struct G; // expected-note {{previous non-type template parameter with type 'int' is here}}
};
template<typename T> struct H {
- // FIXME: As with cases above, the note here is on an unhelpful declaration,
- // and should point to the declaration of G within F.
template<T> friend struct F::G; // \
- // expected-error {{different type 'char' in template redeclaration}} \
- // expected-note {{previous}}
+ // expected-error {{different type 'char' in template redeclaration}}
};
H<int> h1; // ok
H<char> h2; // expected-note {{instantiation}}
More information about the cfe-commits
mailing list