[PATCH] D37308: Fix the __interface inheritence rules to work better with IUnknown and IDispatch

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: lib/AST/DeclCXX.cpp:1474
+bool CXXRecordDecl::isInterfaceLike() const {
+  // All __interfaces are inheritently interface like.
+  if (isInterface())
----------------
nit: use "interface-like" for consistency with the comments below


================
Comment at: lib/AST/DeclCXX.cpp:1477
+    return true;
+
+  // No interface-like types can have a user declared constructor, destructor,
----------------
What should this do for incomplete types, i.e. forward decls and templates undergoing instantiation? The only caller of this checks for this case before hand, but maybe we should `assert(hasDefinition());` on entry.


================
Comment at: lib/AST/DeclCXX.cpp:1478
+
+  // No interface-like types can have a user declared constructor, destructor,
+  // friends, VBases, conersion functions, or fields.  Additionally, lambdas
----------------
Maybe "Interface-like types cannot have ..."

Can an interface-like type have an explicitly defaulted copy ctor? That will be user declared, but it will probably be trivial.


================
Comment at: lib/AST/DeclCXX.cpp:1486-1489
+  // No interface-like type can have a method with a definition.
+  for (const auto *const Method : methods())
+    if (Method->isDefined())
+      return false;
----------------
Huh, so they can be declared, but they don't have to be pure.


================
Comment at: lib/AST/DeclCXX.cpp:1516-1517
+    const auto *Base = BS.getType()->getAsCXXRecordDecl();
+    if (Base->isInterface())
+      continue;
+    if (Base->isInterfaceLike()) {
----------------
Are you sure you want this? MSVC rejects the test case that exercises this case.


================
Comment at: test/SemaCXX/ms-iunknown-outofline-def.cpp:9-10
+void IUnknown::foo() {}
+// expected-error at +1{{interface type cannot inherit from}}
+__interface HasError : public IUnknown {}; 
----------------
Again, this is a pretty weird error. If the method isn't pure, there must be a definition *somewhere* in the program, unless interface types are always declared with novtable.


================
Comment at: test/SemaCXX/ms-iunknown.cpp:24
+
+struct fine_base : temp_iface, IUnknown {};
+__interface can_inherit: public fine_base{};
----------------
MSVC seems to reject this use of multiple inheritance. Are you sure it doesn't just require single-inheritance from an interface-like type? If so, maybe you can simplify the loop over the bases?


https://reviews.llvm.org/D37308