[PATCH] D37308: Fix the __interface inheritence rules to work better with IUnknown and IDispatch
rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D37308 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37308: Fix the __interface inheritence rules to work better with IUnknown and IDispatch
erichkeane updated this revision to Diff 115272.
erichkeane marked an inline comment as done.
erichkeane added a comment.
Fixed for @rnk s comments.
https://reviews.llvm.org/D37308
Files:
include/clang/AST/DeclCXX.h
lib/AST/DeclCXX.cpp
lib/Sema/SemaDeclCXX.cpp
test/SemaCXX/ms-iunknown-inline-def.cpp
test/SemaCXX/ms-iunknown-outofline-def.cpp
test/SemaCXX/ms-iunknown.cpp
Index: test/SemaCXX/ms-iunknown.cpp
===
--- /dev/null
+++ test/SemaCXX/ms-iunknown.cpp
@@ -0,0 +1,39 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-extensions %s
+
+struct __declspec(uuid("---C000-0046")) IUnknown {
+ void foo();
+};
+struct IPropertyPageBase : public IUnknown {};
+struct IPropertyPage : public IPropertyPageBase {};
+__interface ISfFileIOPropertyPage : public IPropertyPage {};
+
+
+namespace NS {
+ struct __declspec(uuid("---C000-0046")) IUnknown {};
+ // expected-error@+1 {{interface type cannot inherit from}}
+ __interface IPropertyPageBase : public IUnknown {};
+}
+// expected-error@+1 {{interface type cannot inherit from}}
+__interface IPropertyPageBase2 : public NS::IUnknown {};
+
+__interface temp_iface {};
+struct bad_base : temp_iface {};
+// expected-error@+1 {{interface type cannot inherit from}}
+__interface bad_inherit : public bad_base{};
+
+struct mult_inher_base : temp_iface, IUnknown {};
+// expected-error@+1 {{interface type cannot inherit from}}
+__interface bad_inherit2 : public mult_inher_base{};
+
+struct PageBase : public IUnknown {};
+struct Page3 : public PageBase {};
+struct Page4 : public PageBase {};
+__interface PropertyPage : public Page4 {};
+
+struct Page5 : public Page3, Page4{};
+// expected-error@+1 {{interface type cannot inherit from}}
+__interface PropertyPage2 : public Page5 {};
+
+__interface IF1 {};
+__interface PP : IUnknown, IF1{};
+__interface PP2 : PP, Page3, Page4{};
Index: test/SemaCXX/ms-iunknown-outofline-def.cpp
===
--- /dev/null
+++ test/SemaCXX/ms-iunknown-outofline-def.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-extensions %s
+
+struct __declspec(uuid("---C000-0046")) IUnknown {
+ void foo();
+};
+
+__interface NoError : public IUnknown {};
+void IUnknown::foo() {}
+// expected-error@+1{{interface type cannot inherit from}}
+__interface HasError : public IUnknown {};
Index: test/SemaCXX/ms-iunknown-inline-def.cpp
===
--- /dev/null
+++ test/SemaCXX/ms-iunknown-inline-def.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-extensions %s
+
+struct __declspec(uuid("---C000-0046")) IUnknown {
+ void foo() {}
+};
+
+// expected-error@+1{{interface type cannot inherit from}}
+__interface HasError : public IUnknown {};
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -2388,7 +2388,7 @@
if (const RecordType *Record = NewBaseType->getAs()) {
const CXXRecordDecl *RD = cast(Record->getDecl());
if (Class->isInterface() &&
- (!RD->isInterface() ||
+ (!RD->isInterfaceLike() ||
KnownBase->getAccessSpecifier() != AS_public)) {
// The Microsoft extension __interface does not permit bases that
// are not themselves public interfaces.
Index: lib/AST/DeclCXX.cpp
===
--- lib/AST/DeclCXX.cpp
+++ lib/AST/DeclCXX.cpp
@@ -1470,6 +1470,53 @@
return false;
}
+bool CXXRecordDecl::isInterfaceLike() const {
+ assert(hasDefinition() && "checking for interface-like without a definition");
+ // All __interfaces are inheritently interface-like.
+ if (isInterface())
+return true;
+
+ // Interface-like types cannot have a user declared constructor, destructor,
+ // friends, VBases, conversion functions, or fields. Additionally, lambdas
+ // cannot be interface types.
+ if (isLambda() || hasUserDeclaredConstructor() ||
+ hasUserDeclaredDestructor() || !field_empty() || hasFriends() ||
+ getNumVBases() > 0 || conversion_end() - conversion_begin() > 0)
+return false;
+
+ // No interface-like type can have a method with a definition.
+ for (const auto *const Method : methods())
+if (Method->isDefined())
+ return false;
+
+ // Check "Special" types.
+ const auto *Uuid = getAttr();
+ if (Uuid && isStruct() && getDeclContext()->isTranslationUnit() &&
+ ((getName() == "IUnknown" &&
+Uuid->getGuid() == "---C000-0046") ||
+ (getName() == "IDispatch" &&
+Uuid->getGuid() == "00020400---C000-0046"))) {
+if (getNumBases() > 0)
+ return false;
+return true;
+ }
+
+ // FIXME: Any
[PATCH] D37308: Fix the __interface inheritence rules to work better with IUnknown and IDispatch
erichkeane marked 4 inline comments as done.
erichkeane added a comment.
Thanks for the quick response! New patch coming soon.
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
rnk wrote:
> 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.
It cannot according to godbolt. https://godbolt.org/g/wZuNzT
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;
rnk wrote:
> Huh, so they can be declared, but they don't have to be pure.
Correct, and strange. Annoyingly, it is possible to do:
struct S : someIFace {
void foo();
};
__interface F : public S {}; // This one is OK.
void S::foo();
__interface F2 : public S {}; // This one is NOT OK.
Comment at: lib/AST/DeclCXX.cpp:1516-1517
+const auto *Base = BS.getType()->getAsCXXRecordDecl();
+if (Base->isInterface())
+ continue;
+if (Base->isInterfaceLike()) {
rnk wrote:
> Are you sure you want this? MSVC rejects the test case that exercises this
> case.
AM I sure? Not anymore. WAS I sure? Pretty darn. Thanks for the catch!
Comment at: test/SemaCXX/ms-iunknown-outofline-def.cpp:9-10
+void IUnknown::foo() {}
+// expected-error@+1{{interface type cannot inherit from}}
+__interface HasError : public IUnknown {};
rnk wrote:
> 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.
Yeah, I agree. I have no idea if I can limit this to just pure functions,
since I'm not totally sure about the use cases. We have at least 1 test in our
testbase that isn't virtual, but the context has been lost (might have been
minimized from another repro).
https://reviews.llvm.org/D37308
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37308: Fix the __interface inheritence rules to work better with IUnknown and IDispatch
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@+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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37308: Fix the __interface inheritence rules to work better with IUnknown and IDispatch
erichkeane updated this revision to Diff 115153.
erichkeane retitled this revision from "Interface class with uuid base record"
to "Fix the __interface inheritence rules to work better with IUnknown and
IDispatch".
erichkeane edited the summary of this revision.
erichkeane added a comment.
Herald added a subscriber: eraman.
Based off of what I discovered previously, this is my first run at a solution
to this problem. @zahiraam (and others), if you can see if I missed anything in
this patch, it would be appreciated. I think this is a good place to start
further discussion than text conversation.
https://reviews.llvm.org/D37308
Files:
include/clang/AST/DeclCXX.h
lib/AST/DeclCXX.cpp
lib/Sema/SemaDeclCXX.cpp
test/SemaCXX/ms-iunknown-inline-def.cpp
test/SemaCXX/ms-iunknown-outofline-def.cpp
test/SemaCXX/ms-iunknown.cpp
Index: test/SemaCXX/ms-iunknown.cpp
===
--- /dev/null
+++ test/SemaCXX/ms-iunknown.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-extensions %s
+
+struct __declspec(uuid("---C000-0046")) IUnknown {
+ void foo();
+};
+struct IPropertyPageBase : public IUnknown {};
+struct IPropertyPage : public IPropertyPageBase {};
+__interface ISfFileIOPropertyPage : public IPropertyPage {};
+
+
+namespace NS {
+ struct __declspec(uuid("---C000-0046")) IUnknown {};
+ // expected-error@+1 {{interface type cannot inherit from}}
+ __interface IPropertyPageBase : public IUnknown {};
+}
+// expected-error@+1 {{interface type cannot inherit from}}
+__interface IPropertyPageBase2 : public NS::IUnknown {};
+
+__interface temp_iface {};
+struct bad_base : temp_iface {};
+// expected-error@+1 {{interface type cannot inherit from}}
+__interface cant_inherit: public bad_base{};
+
+struct fine_base : temp_iface, IUnknown {};
+__interface can_inherit: public fine_base{};
+
+struct PageBase : public IUnknown {};
+struct Page3 : public PageBase {};
+struct Page4 : public PageBase {};
+__interface PropertyPage : public Page4 {};
+
+struct Page5 : public Page3, Page4{};
+// expected-error@+1 {{interface type cannot inherit from}}
+__interface PropertyPage2 : public Page5 {};
+
+__interface IF1 {};
+__interface PP : IUnknown, IF1{};
+__interface PP2 : PP, Page3, Page4{};
Index: test/SemaCXX/ms-iunknown-outofline-def.cpp
===
--- /dev/null
+++ test/SemaCXX/ms-iunknown-outofline-def.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-extensions %s
+
+struct __declspec(uuid("---C000-0046")) IUnknown {
+ void foo();
+};
+
+__interface NoError : public IUnknown {};
+void IUnknown::foo() {}
+// expected-error@+1{{interface type cannot inherit from}}
+__interface HasError : public IUnknown {};
Index: test/SemaCXX/ms-iunknown-inline-def.cpp
===
--- /dev/null
+++ test/SemaCXX/ms-iunknown-inline-def.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-extensions %s
+
+struct __declspec(uuid("---C000-0046")) IUnknown {
+ void foo() {}
+};
+
+// expected-error@+1{{interface type cannot inherit from}}
+__interface HasError : public IUnknown {};
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -2388,7 +2388,7 @@
if (const RecordType *Record = NewBaseType->getAs()) {
const CXXRecordDecl *RD = cast(Record->getDecl());
if (Class->isInterface() &&
- (!RD->isInterface() ||
+ (!RD->isInterfaceLike() ||
KnownBase->getAccessSpecifier() != AS_public)) {
// The Microsoft extension __interface does not permit bases that
// are not themselves public interfaces.
Index: lib/AST/DeclCXX.cpp
===
--- lib/AST/DeclCXX.cpp
+++ lib/AST/DeclCXX.cpp
@@ -1470,6 +1470,61 @@
return false;
}
+bool CXXRecordDecl::isInterfaceLike() const {
+ // All __interfaces are inheritently interface like.
+ if (isInterface())
+return true;
+
+ // No interface-like types can have a user declared constructor, destructor,
+ // friends, VBases, conersion functions, or fields. Additionally, lambdas
+ // cannot be interface types.
+ if (isLambda() || hasUserDeclaredConstructor() ||
+ hasUserDeclaredDestructor() || !field_empty() || hasFriends() ||
+ getNumVBases() > 0 || conversion_end() - conversion_begin() > 0)
+return false;
+
+ // No interface-like type can have a method with a definition.
+ for (const auto *const Method : methods())
+if (Method->isDefined())
+ return false;
+
+ // Check "Special" types.
+ const auto *Uuid = getAttr();
+ if (Uuid && isStruct() && getDeclContext()->isTranslationUnit() &&
+
