<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Nov 9, 2012, at 6:53 PM, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>This isn't actually a circular dependence, either. IntervalMap<>::const_iterator is inheriting from std::iterator.</div><div><br></div><div>I don't see anything obviously wrong with your patch, though...</div></div></blockquote><div><br></div>I see the problem in my patch. CXXRecordDecl::forallBases has a ridiculously non-intuitive API.</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre">     </span>- Doug</div><div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Nov 9, 2012, at 18:21 , Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>LLVM itself seems to have circular inheritance. Is this supposed to happen?</div><div><br></div><div><pre style="font-family: 'Courier New', courier, monotype; "><span class="stderr" style="color: red; ">In file included from /Users/buildslave/zorg/buildbot/smooshlab/slave-0.8/build.clang-x86_64-darwin10-DA/llvm/lib/Support/IntervalMap.cpp:14:
/Users/buildslave/zorg/buildbot/smooshlab/slave-0.8/build.clang-x86_64-darwin10-DA/llvm/include/llvm/ADT/IntervalMap.h:1499:61: error: circular inheritance between 'llvm::IntervalMap::const_iterator' and 'llvm::IntervalMap::iterator'
class IntervalMap<KeyT, ValT, N, Traits>::iterator : public const_iterator {
                                                            ^
/Users/buildslave/zorg/buildbot/smooshlab/slave-0.8/build.clang-x86_64-darwin10-DA/llvm/include/llvm/ADT/IntervalMap.h:1275:43: note: 'llvm::IntervalMap::const_iterator' declared here
class IntervalMap<KeyT, ValT, N, Traits>::const_iterator :
                                          ^
</span></pre></div><div>(This is from smooshlab.)</div><div><br></div><br><div><div>On Nov 9, 2012, at 17:18 , Douglas Gregor <<a href="mailto:dgregor@apple.com">dgregor@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Author: dgregor<br>Date: Fri Nov  9 19:18:17 2012<br>New Revision: 167651<br><br>URL: <a href="http://llvm.org/viewvc/llvm-project?rev=167651&view=rev">http://llvm.org/viewvc/llvm-project?rev=167651&view=rev</a><br>Log:<br>Diagnostic circular inheritance involving dependent base classes. We<br>would have diagnosed this at instantiation time anyway, if only we<br>didn't hang on all of these test cases. Fixes <<a href="rdar://problem/12629723">rdar://problem/12629723</a>><br><br>Modified:<br>    cfe/trunk/include/clang/AST/DeclCXX.h<br>    cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>    cfe/trunk/lib/AST/CXXInheritance.cpp<br>    cfe/trunk/lib/Sema/SemaDeclCXX.cpp<br>    cfe/trunk/test/SemaTemplate/dependent-names.cpp<br><br>Modified: cfe/trunk/include/clang/AST/DeclCXX.h<br>URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=167651&r1=167650&r2=167651&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=167651&r1=167650&r2=167651&view=diff</a><br>==============================================================================<br>--- cfe/trunk/include/clang/AST/DeclCXX.h (original)<br>+++ cfe/trunk/include/clang/AST/DeclCXX.h Fri Nov  9 19:18:17 2012<br>@@ -1322,8 +1322,12 @@<br>   /// \param AllowShortCircuit if false, forces the callback to be called<br>   /// for every base class, even if a dependent or non-matching base was<br>   /// found.<br>+  ///<br>+  /// \param VisitDependent whether we should also visit dependent bases<br>+  /// that can be resolved to CXXRecordDecls.<br>   bool forallBases(ForallBasesCallback *BaseMatches, void *UserData,<br>-                   bool AllowShortCircuit = true) const;<br>+                   bool AllowShortCircuit = true,<br>+                   bool VisitDependent = false) const;<br><br>   /// \brief Function type used by lookupInBases() to determine whether a<br>   /// specific base class subobject matches the lookup criteria.<br><br>Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=167651&r1=167650&r2=167651&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=167651&r1=167650&r2=167651&view=diff</a><br>==============================================================================<br>--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Nov  9 19:18:17 2012<br>@@ -5275,6 +5275,8 @@<br> def err_base_clause_on_union : Error<"unions cannot have base classes">;<br> def err_base_must_be_class : Error<"base specifier must name a class">;<br> def err_union_as_base_class : Error<"unions cannot be base classes">;<br>+def err_circular_inheritance : Error<<br>+  "circular inheritance between %0 and %1">;<br> def err_incomplete_base_class : Error<"base class has incomplete type">;<br> def err_duplicate_base_class : Error<<br>   "base class %0 specified more than once as a direct base class">;<br><br>Modified: cfe/trunk/lib/AST/CXXInheritance.cpp<br>URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/CXXInheritance.cpp?rev=167651&r1=167650&r2=167651&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/CXXInheritance.cpp?rev=167651&r1=167650&r2=167651&view=diff</a><br>==============================================================================<br>--- cfe/trunk/lib/AST/CXXInheritance.cpp (original)<br>+++ cfe/trunk/lib/AST/CXXInheritance.cpp Fri Nov  9 19:18:17 2012<br>@@ -123,7 +123,8 @@<br><br> bool CXXRecordDecl::forallBases(ForallBasesCallback *BaseMatches,<br>                                 void *OpaqueData,<br>-                                bool AllowShortCircuit) const {<br>+                                bool AllowShortCircuit,<br>+                                bool VisitDependent) const {<br>   SmallVector<const CXXRecordDecl*, 8> Queue;<br><br>   const CXXRecordDecl *Record = this;<br>@@ -131,15 +132,14 @@<br>   while (true) {<br>     for (CXXRecordDecl::base_class_const_iterator<br>            I = Record->bases_begin(), E = Record->bases_end(); I != E; ++I) {<br>-      const RecordType *Ty = I->getType()->getAs<RecordType>();<br>-      if (!Ty) {<br>+      CXXRecordDecl *Base = I->getType()->getAsCXXRecordDecl();<br>+      if (!Base || (!VisitDependent && I->getType()->isDependentType())) {<br>         if (AllowShortCircuit) return false;<br>         AllMatches = false;<br>         continue;<br>       }<br><br>-      CXXRecordDecl *Base = <br>-            cast_or_null<CXXRecordDecl>(Ty->getDecl()->getDefinition());<br>+      Base = Base->getDefinition();<br>       if (!Base) {<br>         if (AllowShortCircuit) return false;<br>         AllMatches = false;<br><br>Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp<br>URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=167651&r1=167650&r2=167651&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=167651&r1=167650&r2=167651&view=diff</a><br>==============================================================================<br>--- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)<br>+++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Fri Nov  9 19:18:17 2012<br>@@ -1018,6 +1018,14 @@<br>     return false;<br> }<br><br>+/// \brief Determine whether we have the same C++ record definition.<br>+///<br>+/// Used as a helper function in Sema::CheckBaseSpecifier, below.<br>+static bool sameCXXRecordDef(const CXXRecordDecl *BaseDefinition,<br>+                          void *UserData) {<br>+  return (CXXRecordDecl *)UserData != BaseDefinition;<br>+}<br>+<br> /// \brief Check the validity of a C++ base class specifier.<br> ///<br> /// \returns a new CXXBaseSpecifier if well-formed, emits diagnostics<br>@@ -1044,13 +1052,32 @@<br>       << TInfo->getTypeLoc().getSourceRange();<br>     EllipsisLoc = SourceLocation();<br>   }<br>-  <br>-  if (BaseType->isDependentType())<br>+<br>+  SourceLocation BaseLoc = TInfo->getTypeLoc().getBeginLoc();<br>+<br>+  if (BaseType->isDependentType()) {<br>+    // Make sure that we don't have circular inheritance among our dependent<br>+    // bases. For non-dependent bases, the check for completeness below handles<br>+    // this.<br>+    if (CXXRecordDecl *BaseDecl = BaseType->getAsCXXRecordDecl()) {<br>+      if (BaseDecl->getCanonicalDecl() == Class->getCanonicalDecl() ||<br>+          ((BaseDecl = BaseDecl->getDefinition()) &&<br>+           !BaseDecl->forallBases(&sameCXXRecordDef, Class))) {<br>+        Diag(BaseLoc, diag::err_circular_inheritance)<br>+          << BaseType << Context.getTypeDeclType(Class);<br>+<br>+        if (BaseDecl->getCanonicalDecl() != Class->getCanonicalDecl())<br>+          Diag(BaseDecl->getLocation(), diag::note_previous_decl)<br>+            << BaseType;<br>+            <br>+        return 0;<br>+      }<br>+    }<br>+<br>     return new (Context) CXXBaseSpecifier(SpecifierRange, Virtual,<br>                                           Class->getTagKind() == TTK_Class,<br>                                           Access, TInfo, EllipsisLoc);<br>-<br>-  SourceLocation BaseLoc = TInfo->getTypeLoc().getBeginLoc();<br>+  }<br><br>   // Base specifiers must be record types.<br>   if (!BaseType->isRecordType()) {<br><br>Modified: cfe/trunk/test/SemaTemplate/dependent-names.cpp<br>URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaTemplate/dependent-names.cpp?rev=167651&r1=167650&r2=167651&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaTemplate/dependent-names.cpp?rev=167651&r1=167650&r2=167651&view=diff</a><br>==============================================================================<br>--- cfe/trunk/test/SemaTemplate/dependent-names.cpp (original)<br>+++ cfe/trunk/test/SemaTemplate/dependent-names.cpp Fri Nov  9 19:18:17 2012<br>@@ -319,8 +319,27 @@<br> template < unsigned > struct X {<br>   static const unsigned dimension = 3;<br>   template<unsigned dim=dimension> <br>-  struct Y: Y<dim> { }; // expected-error {{incomplete type}} expected-note {{is not complete until the closing}}<br>+  struct Y: Y<dim> { }; // expected-error{{circular inheritance between 'Y<dim>' and 'Y<dim>'}}<br> };<br> typedef X<3> X3;<br>-X3::Y<>::iterator it; // expected-note {{requested here}}<br>+X3::Y<>::iterator it; // expected-error {{no type named 'iterator' in 'PR11421::X<3>::Y<3>'}}<br>+}<br>+<br>+namespace rdar12629723 {<br>+  template<class T><br>+  struct X {<br>+    struct C : public C { }; // expected-error{{circular inheritance between 'rdar12629723::X::C' and 'rdar12629723::X::C'}}<br>+<br>+    struct B;<br>+<br>+    struct A : public B {  // expected-note{{'rdar12629723::X::A' declared here}}<br>+      virtual void foo() { }<br>+    };<br>+    struct B;<br>+  };<br>+<br>+  template<class T><br>+  struct X<T>::B : public A {  // expected-error{{circular inheritance between 'rdar12629723::X::A' and 'rdar12629723::X::B'}}<br>+    virtual void foo() { }<br>+  };<br> }<br><br><br>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br></blockquote></div><br></div></blockquote></div><br></div></blockquote></div><br></body></html>