Skip to content

Commit

Permalink
Reapply "[clang] Fix name lookup for dependent bases" (llvm#118003)
Browse files Browse the repository at this point in the history
Unlike the previous version
(llvm#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.
  • Loading branch information
vbe-sc authored Dec 3, 2024
1 parent 2a0ee09 commit e1cb316
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 11 deletions.
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,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
------------------

Expand Down
18 changes: 12 additions & 6 deletions clang/lib/AST/CXXInheritance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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))) {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand All @@ -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)) {
Expand Down
2 changes: 0 additions & 2 deletions clang/lib/AST/DeclCXX.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
48 changes: 46 additions & 2 deletions clang/test/CXX/drs/cwg5xx.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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: 20
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'}}
};
Expand Down
2 changes: 1 addition & 1 deletion clang/www/cxx_dr_status.html
Original file line number Diff line number Diff line change
Expand Up @@ -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="unreleased" align="center">Clang 20</td>
</tr>
<tr id="592">
<td><a href="https://cplusplus.github.io/CWG/issues/592.html">592</a></td>
Expand Down

0 comments on commit e1cb316

Please sign in to comment.