[Clang] Fix name lookup for dependent bases #114978
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Vladislav Belov (vbe-sc) ChangesCurrently the following example is a compilation failure: According to the point 13.8.3.2 The base class This patch makes this example compile Full diff: https://github.com/llvm/llvm-project/pull/114978.diff 2 Files Affected:
diff --git a/clang/lib/AST/CXXInheritance.cpp b/clang/lib/AST/CXXInheritance.cpp
index eb265a872c1259..049532f942d051 100644
--- a/clang/lib/AST/CXXInheritance.cpp
+++ b/clang/lib/AST/CXXInheritance.cpp
@@ -169,6 +169,9 @@ bool CXXBasePaths::lookupInBases(ASTContext &Context,
// Find the record of the base class subobjects for this type.
QualType BaseType =
Context.getCanonicalType(BaseSpec.getType()).getUnqualifiedType();
+ bool isCurrentInstantiation = false;
+ if (auto *TST = BaseSpec.getType()->getAs<TemplateSpecializationType>())
+ isCurrentInstantiation = TST->isCurrentInstantiation();
// C++ [temp.dep]p3:
// In the definition of a class template or a member of a class template,
@@ -176,7 +179,8 @@ bool CXXBasePaths::lookupInBases(ASTContext &Context,
// the base class scope is not examined during unqualified name lookup
// either at the point of definition of the class template or member or
// during an instantiation of the class tem- plate or member.
- if (!LookupInDependent && BaseType->isDependentType())
+ if (!LookupInDependent &&
+ (BaseType->isDependentType() && !isCurrentInstantiation))
continue;
// Determine whether we need to visit this base class at all,
@@ -244,9 +248,8 @@ bool CXXBasePaths::lookupInBases(ASTContext &Context,
return FoundPath;
}
} else if (VisitBase) {
- CXXRecordDecl *BaseRecord;
+ CXXRecordDecl *BaseRecord = nullptr;
if (LookupInDependent) {
- BaseRecord = nullptr;
const TemplateSpecializationType *TST =
BaseSpec.getType()->getAs<TemplateSpecializationType>();
if (!TST) {
@@ -265,8 +268,8 @@ bool CXXBasePaths::lookupInBases(ASTContext &Context,
BaseRecord = nullptr;
}
} else {
- BaseRecord = cast<CXXRecordDecl>(
- BaseSpec.getType()->castAs<RecordType>()->getDecl());
+ if (auto *RT = BaseSpec.getType()->getAs<RecordType>())
+ BaseRecord = cast<CXXRecordDecl>(RT->getDecl());
}
if (BaseRecord &&
lookupInBases(Context, BaseRecord, BaseMatches, LookupInDependent)) {
diff --git a/clang/test/CXX/drs/cwg5xx.cpp b/clang/test/CXX/drs/cwg5xx.cpp
index ed0c7159dfc889..b283684aef2f7e 100644
--- a/clang/test/CXX/drs/cwg5xx.cpp
+++ b/clang/test/CXX/drs/cwg5xx.cpp
@@ -1178,17 +1178,21 @@ namespace cwg590 { // cwg590: yes
template<typename T> typename A<T>::B::C A<T>::B::C::f(A<T>::B::C) {}
}
-namespace cwg591 { // cwg591: no
+namespace cwg591 { // cwg591: yes
template<typename T> struct A {
typedef int M;
struct B {
typedef void M;
struct C;
+ struct D;
};
};
template<typename T> struct A<T>::B::C : A<T> {
- // FIXME: Should find member of non-dependent base class A<T>.
+ M m;
+ };
+
+ template<typename T> struct A<T>::B::D : A<T*> {
M m;
// expected-error@-1 {{field has incomplete type 'M' (aka 'void'}}
};
|
vbe-sc
force-pushed
the
vb-sc/clang-dependent-lookup-fix
branch
from
4eb7be6 to
0c2363c
Compare
|
@Endilll, please, could you take a look? |
It seems to be unrelated to you. Might be fixed when you'll push a new commit. |
My knowledge of wording about templates is not deep enough, unfortunately. |
Could you, please, suggest someone who can review this part? |
I've already added several people who should be able to, but mind that we're very short on reviewers, so this will likely take time. |
vbe-sc
force-pushed
the
vb-sc/clang-dependent-lookup-fix
branch
from
0c2363c to
66f3465
Compare
|
@sdkrystian, please, could you take a look? |
There was a problem hiding this comment.
LGTM, but give some time for @sdkrystian to take a look, since he is actively working on this area.
|
@sdkrystian, could you please take a look? |
|
@vbe-sc I can later today or tomorrow. |
That's would be great, thank you! |
|
@sdkrystian ping |
|
@sdkrystian, could you, please, take a look? We are about to merge this |
|
Please wait for a week before pinging someone again. I think this is fine to merge as is, if you want to go ahead. We can always do post commit review later. |
Sorry, I didn't know. |
There was a problem hiding this comment.
I don't think this patch fixes the following case:
template<typename T>
struct A
{
struct B
{
using X = int;
struct C
{
using X = void;
struct D;
};
};
};
template<typename T>
struct A<T>::B::C::D : B
{
X x; // error: field has incomplete type 'X' (aka 'void')
};Regardless, this patch should probably include this as a test.
vbe-sc
force-pushed
the
vb-sc/clang-dependent-lookup-fix
branch
from
66f3465 to
0de2ff2
Compare
Many thanks for your answer! You're right, this test failed with this patch, but I've already fixed it (see the new changes). |
vbe-sc
force-pushed
the
vb-sc/clang-dependent-lookup-fix
branch
2 times, most recently
from
2cea79d to
f3b8ee9
Compare
vbe-sc
force-pushed
the
vb-sc/clang-dependent-lookup-fix
branch
from
f3b8ee9 to
5a48960
Compare
clang/lib/AST/CXXInheritance.cpp
Outdated
| @@ -170,13 +170,21 @@ bool CXXBasePaths::lookupInBases(ASTContext &Context, | |||
| QualType BaseType = | |||
| Context.getCanonicalType(BaseSpec.getType()).getUnqualifiedType(); | |||
|
|
|||
| bool isCurrentInstantiation = isa<InjectedClassNameType>(BaseType); | |||
| if (!isCurrentInstantiation) { | |||
| if (auto *BaseRecord = cast_or_null<CXXRecordDecl>( | |||
There was a problem hiding this comment.
| if (auto *BaseRecord = cast_or_null<CXXRecordDecl>( | |
| if (auto *BaseRecord = cast_if_present<CXXRecordDecl>( |
Can this actually be null?
There was a problem hiding this comment.
Yes, you are right. Fixed here and in the other part of the code because there is the same case.
vbe-sc
force-pushed
the
vb-sc/clang-dependent-lookup-fix
branch
2 times, most recently
from
2f7c763 to
e2e4907
Compare
vbe-sc
force-pushed
the
vb-sc/clang-dependent-lookup-fix
branch
from
e2e4907 to
d17588a
Compare
I added a release note as you asked |
|
@sdkrystian, can you, please, re-review this patch? It seems like I fixed your test |
|
@cor3ntin could you, please, re-review this patch and resolve the comments if everything is ok? |
clang/docs/ReleaseNotes.rst
Outdated
| @@ -268,6 +268,9 @@ Resolutions to C++ Defect Reports | |||
| by default. | |||
| (`CWG2521: User-defined literals and reserved identifiers <https://cplusplus.github.io/CWG/issues/2521.html>`_). | |||
|
|
|||
| - Clang now make correct name lookup when dependent base class is the current instantiation. | |||
There was a problem hiding this comment.
| - Clang now make correct name lookup when dependent base class is the current instantiation. | |
| - Fix name lookup for a dependent base class that is the current instantiation. |
There was a problem hiding this comment.
Could we merge this?
vbe-sc
force-pushed
the
vb-sc/clang-dependent-lookup-fix
branch
from
d17588a to
6564e20
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/134/builds/9282 Here is the relevant piece of the build log for the reference |
This reverts commit 4866447 as requested by the commit author. Buildbots fail: * https://lab.llvm.org/buildbot/#/builders/164/builds/4945 * https://lab.llvm.org/buildbot/#/builders/52/builds/4021
This reverts commit 4866447 as requested by the commit author. Buildbots fail: * https://lab.llvm.org/buildbot/#/builders/164/builds/4945 * https://lab.llvm.org/buildbot/#/builders/52/builds/4021
|
FYI this seems to have significant cost for some types of C++ code, in particular clang build time regresses by 0.35% (https://llvm-compile-time-tracker.com/compare.php?from=4a7b56e6e7dd0f83c379ad06b6e81450bc691ba6&to=486644723038555a224fd09d462bb5099e64809e&stat=instructions:u). |
|
Hmm, that compile time regression is unfortunate. @vbe-sc as a part of your next version of this patch, can you do some sort of analysis why this would result in further lookups/instantiation/etc? I could comprehend that perhaps we're skipping the 'current instantiation' now for these bases so now there is more work to do in instantiations (of which there are more of those than the base, sort of out of necessity), but would like some sort of analysis/confirmation that is the case/what is happening in some of those 'worst' comparisons. |
|
Such an example causes a failure in my patch template <class T>
class X {
public:
X() = default;
virtual ~X() = default;
virtual int foo(int x, int y, T &entry) = 0;
void bar() {
struct Y : public X<T> {
Y() : X() {}
int foo(int, int, T &) override {
return 42;
}
};
}
};The output The problem case is class definition with overrided metod. |
void CXXMethodDecl::addOverriddenMethod(const CXXMethodDecl *MD) {
assert(MD->isCanonicalDecl() && "Method is not canonical!");
assert(!MD->getParent()->isDependentContext() &&
"Can't add an overridden method to a class template!");It seems to me that the second assertion is not really a requirement. @mizvekov , can you please comment on this statement? |
Unlike the previous version (#114978), this patch also removes an unnecessary assert that causes Clang to crash when compiling such tests. (clang/lib/AST/DeclCXX.cpp) https://lab.llvm.org/buildbot/#/builders/52/builds/4021 ```c++ template <class T> class X { public: X() = default; virtual ~X() = default; virtual int foo(int x, int y, T &entry) = 0; void bar() { struct Y : public X<T> { Y() : X() {} int foo(int, int, T &) override { return 42; } }; } }; ``` the assertions: ```c++ llvm-project/clang/lib/AST/DeclCXX.cpp:2508: void clang::CXXMethodDecl::addOverriddenMethod(const CXXMethodDecl *): Assertion `!MD->getParent()->isDependentContext() && "Can't add an overridden method to a class template!"' failed. ``` I believe that this assert is unnecessary and contradicts the logic of this patch. After its removal, Clang was successfully built using itself, and all tests passed.
Currently the following example is a compilation failure:
According to the point 13.8.3.2
The base class
Ais the current instantiation, becauseCis a nested class for an enclosing classA<T>, it's is the not-dependent base class and we need to search the names through its scope.This patch makes this example compile