Reapply "[clang] Fix name lookup for dependent bases" #118003
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Vladislav Belov (vbe-sc) ChangesUnlike the previous version, 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 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: 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. Full diff: https://github.com/llvm/llvm-project/pull/118003.diff 5 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 2ecd19bdc39448..c3bb3ea69b03ea 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -308,6 +308,9 @@ Resolutions to C++ Defect Reports
by default.
(`CWG2521: User-defined literals and reserved identifiers <https://cplusplus.github.io/CWG/issues/2521.html>`_).
+- Fix name lookup for a dependent base class that is the current instantiation.
+ (`CWG591: When a dependent base class is the current instantiation <https://cplusplus.github.io/CWG/issues/591.html>`_).
+
C Language Changes
------------------
diff --git a/clang/lib/AST/CXXInheritance.cpp b/clang/lib/AST/CXXInheritance.cpp
index aefc06e9197cfb..10b8d524ff8978 100644
--- a/clang/lib/AST/CXXInheritance.cpp
+++ b/clang/lib/AST/CXXInheritance.cpp
@@ -134,7 +134,7 @@ bool CXXRecordDecl::forallBases(ForallBasesCallback BaseMatches) const {
return false;
CXXRecordDecl *Base =
- cast_or_null<CXXRecordDecl>(Ty->getDecl()->getDefinition());
+ cast_if_present<CXXRecordDecl>(Ty->getDecl()->getDefinition());
if (!Base ||
(Base->isDependentContext() &&
!Base->isCurrentInstantiation(Record))) {
@@ -169,13 +169,21 @@ bool CXXBasePaths::lookupInBases(ASTContext &Context,
QualType BaseType =
Context.getCanonicalType(BaseSpec.getType()).getUnqualifiedType();
+ bool isCurrentInstantiation = isa<InjectedClassNameType>(BaseType);
+ if (!isCurrentInstantiation) {
+ if (auto *BaseRecord = cast_if_present<CXXRecordDecl>(
+ BaseSpec.getType()->getAsRecordDecl()))
+ isCurrentInstantiation = BaseRecord->isDependentContext() &&
+ BaseRecord->isCurrentInstantiation(Record);
+ }
// C++ [temp.dep]p3:
// In the definition of a class template or a member of a class template,
// if a base class of the class template depends on a template-parameter,
// 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,
@@ -243,9 +251,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) {
@@ -264,8 +271,7 @@ bool CXXBasePaths::lookupInBases(ASTContext &Context,
BaseRecord = nullptr;
}
} else {
- BaseRecord = cast<CXXRecordDecl>(
- BaseSpec.getType()->castAs<RecordType>()->getDecl());
+ BaseRecord = cast<CXXRecordDecl>(BaseSpec.getType()->getAsRecordDecl());
}
if (BaseRecord &&
lookupInBases(Context, BaseRecord, BaseMatches, LookupInDependent)) {
diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
index f2f2835641245e..af73c658d6a0c5 100644
--- a/clang/lib/AST/DeclCXX.cpp
+++ b/clang/lib/AST/DeclCXX.cpp
@@ -2602,8 +2602,6 @@ bool CXXMethodDecl::isMoveAssignmentOperator() const {
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!");
assert(MD->isVirtual() && "Method is not virtual!");
getASTContext().addOverriddenMethod(this, MD);
diff --git a/clang/test/CXX/drs/cwg5xx.cpp b/clang/test/CXX/drs/cwg5xx.cpp
index ed0c7159dfc889..0d53a9d07d76de 100644
--- a/clang/test/CXX/drs/cwg5xx.cpp
+++ b/clang/test/CXX/drs/cwg5xx.cpp
@@ -1178,17 +1178,61 @@ 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 G {
+ struct B {
+ typedef int M;
+ struct C {
+ typedef void M;
+ struct D;
+ };
+ };
+ };
+
+ template<typename T> struct H {
+ template<typename U> struct B {
+ typedef int M;
+ template<typename F> struct C {
+ typedef void M;
+ struct D;
+ struct P;
+ };
};
};
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 G<T>::B::C::D : B {
+ M m;
+ };
+
+ template<typename T>
+ template<typename U>
+ template<typename F>
+ struct H<T>::B<U>::C<F>::D : B<U> {
+ M m;
+ };
+
+ template<typename T> struct A<T>::B::D : A<T*> {
+ M m;
+ // expected-error@-1 {{field has incomplete type 'M' (aka 'void'}}
+ };
+
+ template<typename T>
+ template<typename U>
+ template<typename F>
+ struct H<T>::B<U>::C<F>::P : B<F> {
M m;
// expected-error@-1 {{field has incomplete type 'M' (aka 'void'}}
};
diff --git a/clang/www/cxx_dr_status.html b/clang/www/cxx_dr_status.html
index 186f7cc0ace546..c773c58fac4d0f 100755
--- a/clang/www/cxx_dr_status.html
+++ b/clang/www/cxx_dr_status.html
@@ -3599,7 +3599,7 @@ <h2 id="cxxdr">C++ defect report implementation status</h2>
<td><a href="https://cplusplus.github.io/CWG/issues/591.html">591</a></td>
<td>CD4</td>
<td>When a dependent base class is the current instantiation</td>
- <td class="none" align="center">No</td>
+ <td class="none" align="center">Yes</td>
</tr>
<tr id="592">
<td><a href="https://cplusplus.github.io/CWG/issues/592.html">592</a></td>
|
|
@erichkeane, @nikic This is the second version of this patch. I’m not entirely sure if my suggestion regarding the redundant assertion is correct, but I’d like to share some thoughts on the clang build time regression: According to the actual implementation of My patch enforces additional lookups through the bases (as required by the C++ standard), so worse performance is expected regardless. It might be possible to limit this algorithm or improve its efficiency, but at the moment, I don’t see a viable way to achieve this. Do you have any thoughts on the assertion I removed? Do you have any suggestions or concern? Thanks |
zwuis
reviewed
Nov 28, 2024
clang/test/CXX/drs/cwg5xx.cpp
Outdated
| @@ -1178,17 +1178,61 @@ 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 | |||
There was a problem hiding this comment.
Suggested change
| namespace cwg591 { // cwg591: yes | |
| namespace cwg591 { // cwg591: 20 |
clang/www/cxx_dr_status.html
Outdated
| @@ -3599,7 +3599,7 @@ <h2 id="cxxdr">C++ defect report implementation status</h2> | |||
| <td><a href="https://cplusplus.github.io/CWG/issues/591.html">591</a></td> | |||
| <td>CD4</td> | |||
| <td>When a dependent base class is the current instantiation</td> | |||
| <td class="none" align="center">No</td> | |||
| <td class="none" align="center">Yes</td> | |||
There was a problem hiding this comment.
Suggested change
| <td class="none" align="center">Yes</td> | |
| <td class="unreleased" align="center">Clang 20</td> |
vbe-sc
force-pushed
the
vb-sc/reapply-clang-dependent-lookup-fix
branch
from
63f64a3 to
98583ff
Compare
zwuis
approved these changes
Nov 29, 2024
|
@cor3ntin, FYI: we reverted the patch you merged before. This is the second attempt |
erichkeane
approved these changes
Dec 2, 2024
There was a problem hiding this comment.
Please give @cor3ntin 48 hrs from now to have a chance to review this before committing.
|
I'm hitting an assertion on some Firefox code after this landed: |
Thanks for this important information. Do you have a small code reproduction for this problem? It would help us to fix it as soon as possible |
|
I was creducing it (well, cvise-ing). Here's a reproducer: |
Thanks, I'll investigate with it |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
the assertions:
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.