[PATCH] D37308: Interface class with uuid base record
erichkeane commandeered this revision. erichkeane edited reviewers, added: zahiraam; removed: erichkeane. erichkeane added a comment. @zahiraam: quickly commendeering, since I have an implementation that I think better explains my discovery than my words. Feel free to commandeer it back later (inside the 'Add Action...' box above the comment at the bottom). 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: Interface class with uuid base record
erichkeane added a comment.
SO, the implementation would likely be (~2489):
if (Class->isInterface() && (KnownBase->getAccessSpecifier() != TTK_public ||
!RD->isInterface() || !RD->isInterfaceLike()) {
with "isInterfaceLike" testing what I outlined above.
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: Interface class with uuid base record
erichkeane added a comment. A bit more research based on a different implementation: First, there are TWO special types, not just IUnknown! There is also "IDispatch" with UUID: "00020400---c000-0046" A type is 'interface like' if: -it has a ZERO virtual inheritance bases -it has AT LEAST 1 'interface-like' base (interfaces are not 'interface_like' here) -it has ZERO non-interface/non-interface-like bases. -it has zero user defined destructors, constructors, operators, or conversions -it has zero DEFINED methods -it has ZERO data members -it does not have 'private' or 'protected' data access specifiers -it does not have any "friend" decls. An __interface can inherit from other __interfaces, or interface-like bases. However, it is limited to only 1 of the latter. SO, in your example, "Page5" loses its interface-like-ness because it has 2 separate interface-like 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: Interface class with uuid base record
erichkeane added a comment.
Yeah, but
__interface IF1 {};
__interface PP : IUnknown, IF1{};
__interface PP2 : PP, Page3, Page4{};
Base PP has siblings here. It seems the rule is even more complex then.
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: Interface class with uuid base record
zahiraam added a comment.
MSVC and xmain compile this:
struct __declspec(uuid("---C000-0046")) IUnknown {};
struct PageBase : public IUnknown {};
struct Page3 : public PageBase {};
struct Page4 : public PageBase {};
__interface PropertyPage : public Page4 {};
But MSVC doesn't compile this:
struct __declspec(uuid("---C000-0046")) IUnknown {};
struct PageBase : public IUnknown {};
struct Page3 : public PageBase {};
struct Page4 : public PageBase {};
struct Page5 : public Page3, Page4{};
__interface PropertyPage : public Page5 {};
So a base of RD that has siblings and inherits from a Unknown is not permitted.
Which means that if the number is siblings are greater than one, we should
fail. I guess the current function doesn't take that into account.
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: Interface class with uuid base record
zahiraam added a comment.
Erich,
The IsOrInheritsFromIUnknown function is needed.
Wh
Comment at: lib/Sema/SemaDeclCXX.cpp:2377
+/// \brief Tests if the __interface base is public.
+static bool IsBasePublicInterface(const CXXRecordDecl *RD,
+ AccessSpecifier spec) {
erichkeane wrote:
> This function isn't testing the 'base', it is testing the actual record decl.
RD is the base of the interface. But I can rename it to IsRDPublicInterface.
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: Interface class with uuid base record
zahiraam updated this revision to Diff 115125.
zahiraam added a comment.
Hi have made all the changes requested.
https://reviews.llvm.org/D37308
Files:
lib/Sema/SemaDeclCXX.cpp
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -2373,6 +2373,40 @@
return true;
}
+/// \brief Tests if RD is a public interface.
+static bool IsDeclPublicInterface(const CXXRecordDecl *RD,
+ AccessSpecifier spec) {
+ return RD->isInterface() && spec == AS_public;
+}
+
+/// \brief Test if record is a uuid for IUnknown.
+/// This is an MS SDK specific type that has a special
+/// behavior in the CL compiler.
+static bool IsIUnknownType(const CXXRecordDecl *RD) {
+ const auto *Uuid = RD->getAttr();
+
+ return Uuid && RD->isStruct() && RD->isEmpty() &&
+ RD->getDeclContext()->isTranslationUnit() &&
+ RD->getName() == "IUnknown" &&
+ Uuid->getGuid() =="---C000-0046";
+}
+
+/// \brief Test if RD or its inherited bases is an IUnknown type.
+static bool IsOrInheritsFromIUnknown(const CXXRecordDecl *RD) {
+ bool IsUnknown = IsIUnknownType(RD);
+ for (CXXRecordDecl::base_class_const_iterator Base = RD->bases_begin(),
+BaseEnd = RD->bases_end();
+ Base != BaseEnd; ++Base) {
+ CXXRecordDecl *BaseChild = Base->getType()->getAsCXXRecordDecl();
+
+ return IsUnknown ||
+ IsOrInheritsFromIUnknown(BaseChild) ||
+ (RD->getNumBases() > 1) &&
+ IsOrInheritsFromIUnknown((CXXRecordDecl*)
BaseChild->getNextDeclInContext());
+ }
+ return IsUnknown;
+}
+
/// Use small set to collect indirect bases. As this is only used
/// locally, there's no need to abstract the small size parameter.
typedef llvm::SmallPtrSet IndirectBaseSet;
@@ -2450,10 +2484,10 @@
if (const RecordType *Record = NewBaseType->getAs()) {
const CXXRecordDecl *RD = cast(Record->getDecl());
if (Class->isInterface() &&
- (!RD->isInterface() ||
- KnownBase->getAccessSpecifier() != AS_public)) {
- // The Microsoft extension __interface does not permit bases that
- // are not themselves public interfaces.
+// The Microsoft extension __interface does not permit bases that
+// are not themselves public interfaces.
+!IsDeclPublicInterface(RD, KnownBase->getAccessSpecifier()) &&
+!IsOrInheritsFromIUnknown(RD)) {
Diag(KnownBase->getLocStart(), diag::err_invalid_base_in_interface)
<< getRecordDiagFromTagKind(RD->getTagKind()) << RD->getName()
<< RD->getSourceRange();
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -2373,6 +2373,40 @@
return true;
}
+/// \brief Tests if RD is a public interface.
+static bool IsDeclPublicInterface(const CXXRecordDecl *RD,
+ AccessSpecifier spec) {
+ return RD->isInterface() && spec == AS_public;
+}
+
+/// \brief Test if record is a uuid for IUnknown.
+/// This is an MS SDK specific type that has a special
+/// behavior in the CL compiler.
+static bool IsIUnknownType(const CXXRecordDecl *RD) {
+ const auto *Uuid = RD->getAttr();
+
+ return Uuid && RD->isStruct() && RD->isEmpty() &&
+ RD->getDeclContext()->isTranslationUnit() &&
+ RD->getName() == "IUnknown" &&
+ Uuid->getGuid() =="---C000-0046";
+}
+
+/// \brief Test if RD or its inherited bases is an IUnknown type.
+static bool IsOrInheritsFromIUnknown(const CXXRecordDecl *RD) {
+ bool IsUnknown = IsIUnknownType(RD);
+ for (CXXRecordDecl::base_class_const_iterator Base = RD->bases_begin(),
+BaseEnd = RD->bases_end();
+ Base != BaseEnd; ++Base) {
+ CXXRecordDecl *BaseChild = Base->getType()->getAsCXXRecordDecl();
+
+ return IsUnknown ||
+ IsOrInheritsFromIUnknown(BaseChild) ||
+ (RD->getNumBases() > 1) &&
+ IsOrInheritsFromIUnknown((CXXRecordDecl*) BaseChild->getNextDeclInContext());
+ }
+ return IsUnknown;
+}
+
/// Use small set to collect indirect bases. As this is only used
/// locally, there's no need to abstract the small size parameter.
typedef llvm::SmallPtrSet IndirectBaseSet;
@@ -2450,10 +2484,10 @@
if (const RecordType *Record = NewBaseType->getAs()) {
const CXXRecordDecl *RD = cast(Record->getDecl());
if (Class->isInterface() &&
- (!RD->isInterface() ||
- KnownBase->getAccessSpecifier() != AS_public)) {
- // The Microsoft extension __interface does not permit bases that
- // are not themselves public interfaces.
+// The Microsoft extension __interface
[PATCH] D37308: Interface class with uuid base record
erichkeane added a comment. Actually... disregard that... the rule is more complex than that. Based on some playing around with MSVC on godbolt, it seems that it actually marks any type that inherits ONLY from interface types or IUnknown as an interface itself. We may be better off doing that instead. Additionally, the implementation of "isIUnknown" is actually more complicated too, since it contains function declarations. Comment at: lib/Sema/SemaDeclCXX.cpp:2388 + + return Uuid && Uuid->getGuid() =="---C000-0046" && + RD->isStruct() && RD->getName() == "IUnknown" && RD->isEmpty() && erichkeane wrote: > Up to @aaron.ballman, but moving the getGuid string check (a massive > string-diff) to 2nd defeats the purpose of moving UUID up front, which is to > put the 'easy' things first. IsTU, isStruct, and isEmpty are likely better. > > Additionally, I'd mentioned it before, this still doesn't check to make sure > that the IUnknown doesn't inherit from anywhere, does it? MSVC also will > error if IUnknown's definition includes an inherited type. Also, "isEmpty" isn't sufficient. IUnknown actually contains functions as long as none have an implementation. (Declarations only). 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: Interface class with uuid base record
erichkeane added a comment.
Tighten up the 'IUnknown' check, and do the check I mentioned above, and I
think this logic is correct. Searching would be required in the positive case,
but this is the negative case.
Comment at: lib/Sema/SemaDeclCXX.cpp:2483
+!IsDeclPublicInterface(RD, KnownBase->getAccessSpecifier()) &&
+!IsOrInheritsFromIUnknown(RD)) {
Diag(KnownBase->getLocStart(), diag::err_invalid_base_in_interface)
So, I just realized... this call here is useless, we got sidetracked because of
the inversion of logic here. You ACTUALLY just care that this base is either a
public interface, OR if it is IUnknown. This logic can be:
if (Class->isInterface() && !IsDeclPublicInterface(...) &&
!IsIUnknownType(...)) {...}
The search doesn't actually need to take place, since the current base COULD
NOT be a valid 'interface' unless it inherited only from interfaces/IUnknown
anyway. Unless I'm missing something, you can delete the function
"IsOrInheritsFromIUnknown" as well.
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: Interface class with uuid base record
erichkeane added a comment.
Also, Craig mentioned to me that naming rules require static fucntions to start
with a lower case letter (not uppercase). Additionally, Variables (like
'result') need to start with a capital letter.
Comment at: lib/Sema/SemaDeclCXX.cpp:2388
+
+ return Uuid && Uuid->getGuid() =="---C000-0046" &&
+ RD->isStruct() && RD->getName() == "IUnknown" && RD->isEmpty() &&
Up to @aaron.ballman, but moving the getGuid string check (a massive
string-diff) to 2nd defeats the purpose of moving UUID up front, which is to
put the 'easy' things first. IsTU, isStruct, and isEmpty are likely better.
Additionally, I'd mentioned it before, this still doesn't check to make sure
that the IUnknown doesn't inherit from anywhere, does it? MSVC also will error
if IUnknown's definition includes an inherited type.
Comment at: lib/Sema/SemaDeclCXX.cpp:2393
+
+/// \brief Test if RD or its inherited bases is an IUnknow type.
+static bool IsOrInheritsFromIUnknown(const CXXRecordDecl *RD) {
Still a misspelled comment.
Comment at: lib/Sema/SemaDeclCXX.cpp:2395
+static bool IsOrInheritsFromIUnknown(const CXXRecordDecl *RD) {
+ bool result = IsIUnknownType(RD);
+ for (const auto *I = RD->bases_begin(), *E = RD->bases_end(); I != E; ++I) {
Since you know the value of this here, there is no reason to waste time with
the below loop. Return immediately.
Comment at: lib/Sema/SemaDeclCXX.cpp:2396
+ bool result = IsIUnknownType(RD);
+ for (const auto *I = RD->bases_begin(), *E = RD->bases_end(); I != E; ++I) {
+const CXXRecordDecl *BB = I->getType()->getAsCXXRecordDecl();
This should use a range-based for.
Comment at: lib/Sema/SemaDeclCXX.cpp:2397
+ for (const auto *I = RD->bases_begin(), *E = RD->bases_end(); I != E; ++I) {
+const CXXRecordDecl *BB = I->getType()->getAsCXXRecordDecl();
+return result || IsOrInheritsFromIUnknown(BB);
What does "BB" stand for? Please use a descriptive name.
Comment at: lib/Sema/SemaDeclCXX.cpp:2398
+const CXXRecordDecl *BB = I->getType()->getAsCXXRecordDecl();
+return result || IsOrInheritsFromIUnknown(BB);
+ }
Two big issues here:
1- This will never allow this loop to hit the second iteration, which it
absolutely needs to do.
2- Again, you're in a situation where you only need to continue the execution
of this function in certain cases.
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: Interface class with uuid base record
zahiraam updated this revision to Diff 114935.
zahiraam added a comment.
Erich, Aaron,
Please review at your convenience.
Thanks.
https://reviews.llvm.org/D37308
Files:
lib/Sema/SemaDeclCXX.cpp
test/SemaCXX/ms-uuid.cpp
Index: test/SemaCXX/ms-uuid.cpp
===
--- test/SemaCXX/ms-uuid.cpp
+++ test/SemaCXX/ms-uuid.cpp
@@ -93,3 +93,32 @@
[uuid("00A0---C000-0049"),
uuid("00A0---C000-0049")] class C10;
}
+
+struct __declspec(uuid("---C000-0046")) IUnknown {};
+struct IPropertyPageBase : public IUnknown {};
+struct IPropertyPage : public IPropertyPageBase {};
+__interface ISfFileIOPropertyPage : public IPropertyPage {};
+
+
+__interface ISfFileIOPropertyPage1 : public IUnknown {};
+
+struct __declspec(uuid("---C000-0045")) IUnknown1 {};
+__interface ISfFileIOPropertyPage2 : public IUnknown1 {}; // expected-error
{{interface type cannot inherit from}}
+
+struct __declspec(uuid("---C000-0046")) IUnknown2 {};
+struct IPropertyPage2 : public IUnknown2 {};
+__interface ISfFileIOPropertyPage3 : public IPropertyPage2 {}; //
expected-error {{interface type cannot inherit from}}
+
+struct IPropertyPage3 : public IUnknown {};
+__interface ISfFileIOPropertyPage4 : public IPropertyPage3 {};
+
+__interface __declspec(dllimport) ISfFileIOPropertyPage33 : public IUnknown {};
+
+struct __declspec(uuid("---C000-0046")) IUnknown4 {};
+__interface ISfFileIOPropertyPage5 : public IUnknown4 {}; // expected-error
{{interface type cannot inherit from}}
+
+class __declspec(uuid("---C000-0046")) IUnknown5 {};
+__interface ISfFileIOPropertyPage6 : public IUnknown5 {}; // expected-error
{{interface type cannot inherit from}}
+
+struct __declspec(dllexport) IUnknown6{};
+__interface foo : public IUnknown6{}; // expected-error {{interface type
cannot inherit from}}
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -2373,6 +2373,33 @@
return true;
}
+/// \brief Tests if RD is a public interface.
+static bool IsDeclPublicInterface(const CXXRecordDecl *RD,
+ AccessSpecifier spec) {
+ return RD->isInterface() && spec == AS_public;
+}
+
+/// \brief Test if record is a uuid for IUnknown.
+/// This is an MS SDK specific type that has a special
+/// behavior in the CL compiler.
+static bool IsIUnknownType(const CXXRecordDecl *RD) {
+ const auto *Uuid = RD->getAttr();
+
+ return Uuid && Uuid->getGuid() =="---C000-0046" &&
+ RD->isStruct() && RD->getName() == "IUnknown" && RD->isEmpty() &&
+ RD->getDeclContext()->isTranslationUnit();
+}
+
+/// \brief Test if RD or its inherited bases is an IUnknow type.
+static bool IsOrInheritsFromIUnknown(const CXXRecordDecl *RD) {
+ bool result = IsIUnknownType(RD);
+ for (const auto *I = RD->bases_begin(), *E = RD->bases_end(); I != E; ++I) {
+const CXXRecordDecl *BB = I->getType()->getAsCXXRecordDecl();
+return result || IsOrInheritsFromIUnknown(BB);
+ }
+ return result;
+}
+
/// Use small set to collect indirect bases. As this is only used
/// locally, there's no need to abstract the small size parameter.
typedef llvm::SmallPtrSet IndirectBaseSet;
@@ -2450,10 +2477,10 @@
if (const RecordType *Record = NewBaseType->getAs()) {
const CXXRecordDecl *RD = cast(Record->getDecl());
if (Class->isInterface() &&
- (!RD->isInterface() ||
- KnownBase->getAccessSpecifier() != AS_public)) {
- // The Microsoft extension __interface does not permit bases that
- // are not themselves public interfaces.
+// The Microsoft extension __interface does not permit bases that
+// are not themselves public interfaces.
+!IsDeclPublicInterface(RD, KnownBase->getAccessSpecifier()) &&
+!IsOrInheritsFromIUnknown(RD)) {
Diag(KnownBase->getLocStart(), diag::err_invalid_base_in_interface)
<< getRecordDiagFromTagKind(RD->getTagKind()) << RD->getName()
<< RD->getSourceRange();
Index: test/SemaCXX/ms-uuid.cpp
===
--- test/SemaCXX/ms-uuid.cpp
+++ test/SemaCXX/ms-uuid.cpp
@@ -93,3 +93,32 @@
[uuid("00A0---C000-0049"),
uuid("00A0---C000-0049")] class C10;
}
+
+struct __declspec(uuid("---C000-0046")) IUnknown {};
+struct IPropertyPageBase : public IUnknown {};
+struct IPropertyPage : public IPropertyPageBase {};
+__interface ISfFileIOPropertyPage : public IPropertyPage {};
+
+
+__interface ISfFileIOPropertyPage1 : public IUnknown {};
+
+struct __declspec(uuid("---C000-0045")) IUnknown1 {};
[PATCH] D37308: Interface class with uuid base record
aaron.ballman added inline comments.
Comment at: lib/Sema/SemaDeclCXX.cpp:2376
+/// \brief Tests if the __interface base is public.
+static bool IsDeclPublicInterface(const CXXRecordDecl *RD,
The comment does not match the behavior of the function.
Comment at: lib/Sema/SemaDeclCXX.cpp:2382
+
+/// \brief Test if record is an uuid Unknown.
+/// This is an MS SDK specific type that has a special
uuid Unknown -> UUID for IUnknown
Comment at: lib/Sema/SemaDeclCXX.cpp:2386
+static bool IsIUnknownType(const CXXRecordDecl *RD) {
+ auto *Uuid = RD->getAttr();
+
This can be `const auto *`
Comment at: lib/Sema/SemaDeclCXX.cpp:2389
+ return RD->isStruct() && RD->getName() == "IUnknown" && RD->isEmpty() &&
+ Uuid && Uuid->getGuid() =="---C000-0046" &&
+ dyn_cast(RD->getDeclContext());
I would move the test for `Uuid` to be one of the first things tested, since
it's the least expensive.
Comment at: lib/Sema/SemaDeclCXX.cpp:2390
+ Uuid && Uuid->getGuid() =="---C000-0046" &&
+ dyn_cast(RD->getDeclContext());
+}
erichkeane wrote:
> @aaron.ballman This logic we'd like you to particularly check on. Does this
> ensure it isn't in a namespace?
>
> Zahira: since you don't need the value, "isa" is more
> appropriate here.
I would skip the casts entirely and use
`RD->getDeclContext()->isTranslationUnit()`
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: Interface class with uuid base record
erichkeane added a comment.
You seem to have had a hard time with the diff tool too... there is an extra
file here that needs to be removed.
Comment at: lib/Sema/SemaDeclCXX.cpp:2390
+ Uuid && Uuid->getGuid() =="---C000-0046" &&
+ dyn_cast(RD->getDeclContext());
+}
@aaron.ballman This logic we'd like you to particularly check on. Does this
ensure it isn't in a namespace?
Zahira: since you don't need the value, "isa" is more
appropriate here.
Comment at: lib/Sema/SemaDeclCXX.cpp:2393
+
+/// \brief Test if any chidren of inheritated base is an IUnknow type.
+static bool AreChildrenOfBaseIUnknown(const CXXRecordDecl *Base)
Misspelled IUnknown here.
Comment at: lib/Sema/SemaDeclCXX.cpp:2394
+/// \brief Test if any chidren of inheritated base is an IUnknow type.
+static bool AreChildrenOfBaseIUnknown(const CXXRecordDecl *Base)
+{
If you do the below function right, this one ends up not being necessary.
Comment at: lib/Sema/SemaDeclCXX.cpp:2405
+/// \brief Test if RD or its inhetited bases is an IUnknow type.
+static bool IsOrInheritsFromIUnknown(const CXXRecordDecl *RD) {
+ const CXXRecordDecl *Base =
This doesn't cover the other children of RD. It checks only the first Base.
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: Interface class with uuid base record
zahiraam updated this revision to Diff 114497.
zahiraam added a comment.
Erich,
Addressed your comments.
Thanks.
https://reviews.llvm.org/D37308
Files:
lib/Sema/SemaDeclCXX.cpp
test/SemaCXX/ms-uuid.cpp
Index: test/SemaCXX/ms-uuid.cpp
===
--- test/SemaCXX/ms-uuid.cpp
+++ test/SemaCXX/ms-uuid.cpp
@@ -93,3 +93,32 @@
[uuid("00A0---C000-0049"),
uuid("00A0---C000-0049")] class C10;
}
+
+struct __declspec(uuid("---C000-0046")) IUnknown {};
+struct IPropertyPageBase : public IUnknown {};
+struct IPropertyPage : public IPropertyPageBase {};
+__interface ISfFileIOPropertyPage : public IPropertyPage {};
+
+
+__interface ISfFileIOPropertyPage1 : public IUnknown {};
+
+struct __declspec(uuid("---C000-0045")) IUnknown1 {};
+__interface ISfFileIOPropertyPage2 : public IUnknown1 {}; // expected-error
{{interface type cannot inherit from}}
+
+struct __declspec(uuid("---C000-0046")) IUnknown2 {};
+struct IPropertyPage2 : public IUnknown2 {};
+__interface ISfFileIOPropertyPage3 : public IPropertyPage2 {}; //
expected-error {{interface type cannot inherit from}}
+
+struct IPropertyPage3 : public IUnknown {};
+__interface ISfFileIOPropertyPage4 : public IPropertyPage3 {};
+
+__interface __declspec(dllimport) ISfFileIOPropertyPage33 : public IUnknown {};
+
+struct __declspec(uuid("---C000-0046")) IUnknown4 {};
+__interface ISfFileIOPropertyPage5 : public IUnknown4 {}; // expected-error
{{interface type cannot inherit from}}
+
+class __declspec(uuid("---C000-0046")) IUnknown5 {};
+__interface ISfFileIOPropertyPage6 : public IUnknown5 {}; // expected-error
{{interface type cannot inherit from}}
+
+struct __declspec(dllexport) IUnknown6{};
+__interface foo : public IUnknown6{}; // expected-error {{interface type
cannot inherit from}}
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -2373,6 +2373,33 @@
return true;
}
+/// \brief Tests if the __interface base is public.
+static bool IsDeclPublicInterface(const CXXRecordDecl *RD,
+ AccessSpecifier spec) {
+ return RD->isInterface() && spec == AS_public;
+}
+
+/// \brief Test if record is an uuid Unknown.
+/// This is an MS SDK specific type that has a special
+/// behavior in the CL compiler.
+static bool IsIUnknownType(const CXXRecordDecl *RD) {
+ auto *Uuid = RD->getAttr();
+
+ return RD->isStruct() && RD->getName() == "IUnknown" && RD->isEmpty() &&
+ Uuid && Uuid->getGuid() =="---C000-0046" &&
+ dyn_cast(RD->getDeclContext());
+}
+
+/// \brief Test if RD or its inhetited bases is an IUnknow type.
+static bool IsOrInheritsFromIUnknown(const CXXRecordDecl *RD) {
+ const CXXRecordDecl *Base =
+ RD->getNumBases() ? RD->bases_begin()->getType()->getAsCXXRecordDecl()
+: nullptr;
+
+ return IsIUnknownType(RD) ||
+ Base && (IsIUnknownType(Base) || IsOrInheritsFromIUnknown(Base));
+}
+
/// Use small set to collect indirect bases. As this is only used
/// locally, there's no need to abstract the small size parameter.
typedef llvm::SmallPtrSet IndirectBaseSet;
@@ -2450,10 +2477,10 @@
if (const RecordType *Record = NewBaseType->getAs()) {
const CXXRecordDecl *RD = cast(Record->getDecl());
if (Class->isInterface() &&
- (!RD->isInterface() ||
- KnownBase->getAccessSpecifier() != AS_public)) {
- // The Microsoft extension __interface does not permit bases that
- // are not themselves public interfaces.
+// The Microsoft extension __interface does not permit bases that
+// are not themselves public interfaces.
+!IsDeclPublicInterface(RD, KnownBase->getAccessSpecifier()) &&
+!IsOrInheritsFromIUnknown(RD)) {
Diag(KnownBase->getLocStart(), diag::err_invalid_base_in_interface)
<< getRecordDiagFromTagKind(RD->getTagKind()) << RD->getName()
<< RD->getSourceRange();
Index: test/SemaCXX/ms-uuid.cpp
===
--- test/SemaCXX/ms-uuid.cpp
+++ test/SemaCXX/ms-uuid.cpp
@@ -93,3 +93,32 @@
[uuid("00A0---C000-0049"),
uuid("00A0---C000-0049")] class C10;
}
+
+struct __declspec(uuid("---C000-0046")) IUnknown {};
+struct IPropertyPageBase : public IUnknown {};
+struct IPropertyPage : public IPropertyPageBase {};
+__interface ISfFileIOPropertyPage : public IPropertyPage {};
+
+
+__interface ISfFileIOPropertyPage1 : public IUnknown {};
+
+struct __declspec(uuid("---C000-0045")) IUnknown1 {};
+__interface ISfFileIOPropertyPage2
[PATCH] D37308: Interface class with uuid base record
erichkeane added a comment. 1 more thing I missed. Comment at: lib/Sema/SemaDeclCXX.cpp:2389 + return RD->isStruct() && RD->getName() == "IUnknown" && RD->isEmpty() && + Uuid && Uuid->getGuid() =="---C000-0046"; +} This also has to ensure that IUnknown doesn't inherit from anyone AND that it is in the global namespace. 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: Interface class with uuid base record
erichkeane requested changes to this revision.
erichkeane added inline comments.
This revision now requires changes to proceed.
Comment at: lib/Sema/SemaDeclCXX.cpp:2377
+/// \brief Tests if the __interface base is public.
+static bool IsRecordPublicInterface(const CXXRecordDecl *RD,
+ AccessSpecifier spec) {
Not sure that 'Record' is the correct word here. You should likely just do
"IsDeclPublicInterface". Additionally (though not necessary to change), the
only requirement for this type is the isInterface, which comes from TagDecl.
Comment at: lib/Sema/SemaDeclCXX.cpp:2385
+/// behavior in the CL compiler.
+static bool IsUnknownType(const CXXRecordDecl *RD) {
+ auto *Uuid = RD->getAttr();
The type name is "IUnknown". This function name (and the comment above) needs
to match this.
Comment at: lib/Sema/SemaDeclCXX.cpp:2393
+/// \brief Test if record and records in inheritance tree is a an Unknown.
+static bool IsOrInheritsFromUnknown(const CXXRecordDecl *RD) {
+ if (RD->getNumBases()) {
This logic is completely wrong. You are trying to test if RD is IUnknown OR if
ANY of its bases are IUnknown (or if they inherit). You absolutely have to
loop over the bases here, which you aren't doing here at all.
Comment at: test/SemaCXX/ms-uuid.cpp:97
+
+struct __declspec(uuid("---C000-0046")) IUnknown {};
+__interface ISfFileIOPropertyPage : public IUnknown {};
Add some should-compile examples that show multiple inheritance. Particularly
where the IUnknown is inherited from the 2nd or later one.
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: Interface class with uuid base record
zahiraam updated this revision to Diff 114428.
zahiraam added a comment.
Responding to Erich 's and Aaron's reviews. Thanks.
https://reviews.llvm.org/D37308
Files:
lib/Sema/SemaDeclCXX.cpp
test/SemaCXX/ms-uuid.cpp
Index: test/SemaCXX/ms-uuid.cpp
===
--- test/SemaCXX/ms-uuid.cpp
+++ test/SemaCXX/ms-uuid.cpp
@@ -93,3 +93,27 @@
[uuid("00A0---C000-0049"),
uuid("00A0---C000-0049")] class C10;
}
+
+struct __declspec(uuid("---C000-0046")) IUnknown {};
+__interface ISfFileIOPropertyPage : public IUnknown {};
+
+struct __declspec(uuid("---C000-0045")) IUnknown1 {};
+__interface ISfFileIOPropertyPage1 : public IUnknown1 {}; // expected-error
{{interface type cannot inherit from}}
+
+struct __declspec(uuid("---C000-0046")) IUnknown2 {};
+struct IPropertyPage2 : public IUnknown2 {};
+__interface ISfFileIOPropertyPage2 : public IPropertyPage2 {}; //
expected-error {{interface type cannot inherit from}}
+
+struct IPropertyPage3 : public IUnknown {};
+__interface ISfFileIOPropertyPage3 : public IPropertyPage3 {};
+
+__interface __declspec(dllimport) ISfFileIOPropertyPage33 : public IUnknown {};
+
+struct __declspec(uuid("---C000-0046")) IUnknown4 {};
+__interface ISfFileIOPropertyPage4 : public IUnknown4 {}; // expected-error
{{interface type cannot inherit from}}
+
+class __declspec(uuid("---C000-0046")) IUnknown5 {};
+__interface ISfFileIOPropertyPage5 : public IUnknown5 {}; // expected-error
{{interface type cannot inherit from}}
+
+struct __declspec(dllexport) IUnknown6{};
+__interface foo : public IUnknown6{}; // expected-error {{interface type
cannot inherit from}}
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -2373,6 +2373,35 @@
return true;
}
+/// \brief Tests if the __interface base is public.
+static bool IsRecordPublicInterface(const CXXRecordDecl *RD,
+ AccessSpecifier spec) {
+ return RD->isInterface() && spec == AS_public;
+}
+
+/// \brief Test if record is an uuid Unknown.
+/// This is an MS SDK specific type that has a special
+/// behavior in the CL compiler.
+static bool IsUnknownType(const CXXRecordDecl *RD) {
+ auto *Uuid = RD->getAttr();
+
+ return RD->isStruct() && RD->getName() == "IUnknown" && RD->isEmpty() &&
+ Uuid && Uuid->getGuid() =="---C000-0046";
+}
+
+/// \brief Test if record and records in inheritance tree is a an Unknown.
+static bool IsOrInheritsFromUnknown(const CXXRecordDecl *RD) {
+ if (RD->getNumBases()) {
+const CXXRecordDecl *Base =
+ RD->bases_begin()->getType()->getAsCXXRecordDecl();
+
+return Base && IsUnknownType(Base) && !IsUnknownType(RD) &&
+ IsOrInheritsFromUnknown(Base);
+ } else {
+return IsUnknownType(RD);
+ }
+}
+
/// Use small set to collect indirect bases. As this is only used
/// locally, there's no need to abstract the small size parameter.
typedef llvm::SmallPtrSet IndirectBaseSet;
@@ -2450,8 +2479,8 @@
if (const RecordType *Record = NewBaseType->getAs()) {
const CXXRecordDecl *RD = cast(Record->getDecl());
if (Class->isInterface() &&
- (!RD->isInterface() ||
- KnownBase->getAccessSpecifier() != AS_public)) {
+!IsRecordPublicInterface(RD, KnownBase->getAccessSpecifier()) &&
+!IsOrInheritsFromUnknown(RD)) {
// The Microsoft extension __interface does not permit bases that
// are not themselves public interfaces.
Diag(KnownBase->getLocStart(), diag::err_invalid_base_in_interface)
Index: test/SemaCXX/ms-uuid.cpp
===
--- test/SemaCXX/ms-uuid.cpp
+++ test/SemaCXX/ms-uuid.cpp
@@ -93,3 +93,27 @@
[uuid("00A0---C000-0049"),
uuid("00A0---C000-0049")] class C10;
}
+
+struct __declspec(uuid("---C000-0046")) IUnknown {};
+__interface ISfFileIOPropertyPage : public IUnknown {};
+
+struct __declspec(uuid("---C000-0045")) IUnknown1 {};
+__interface ISfFileIOPropertyPage1 : public IUnknown1 {}; // expected-error {{interface type cannot inherit from}}
+
+struct __declspec(uuid("---C000-0046")) IUnknown2 {};
+struct IPropertyPage2 : public IUnknown2 {};
+__interface ISfFileIOPropertyPage2 : public IPropertyPage2 {}; // expected-error {{interface type cannot inherit from}}
+
+struct IPropertyPage3 : public IUnknown {};
+__interface ISfFileIOPropertyPage3 : public IPropertyPage3 {};
+
+__interface __declspec(dllimport) ISfFileIOPropertyPage33 : public IUnknown {};
+
+struct
[PATCH] D37308: Interface class with uuid base record
aaron.ballman added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:2385 + return RD->isStruct() && RD->hasAttr() && + RD->getName() == "IUnknown" && + (RD->getAttr())->getGuid() == This should probably also ensure that the class is in the global namespace so we don't handle `foobar::IUnknown` improperly. Comment at: lib/Sema/SemaDeclCXX.cpp:2386 + RD->getName() == "IUnknown" && + (RD->getAttr())->getGuid() == + "---C000-0046" && Can remove the spurious parens. Also, rather than call `hasAttr<>` followed by `getAttr<>` on the same thing, you should factor out the call to `getAttr<>`. 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: Interface class with uuid base record
erichkeane added inline comments.
Comment at: lib/Sema/SemaDeclCXX.cpp:2377
+/// \brief Tests if the __interface base is public.
+static bool IsBasePublicInterface(const CXXRecordDecl *RD,
+ AccessSpecifier spec) {
This function isn't testing the 'base', it is testing the actual record decl.
Comment at: lib/Sema/SemaDeclCXX.cpp:2391
+
+static bool IsInheritatedBaseUuid(const CXXRecordDecl *RD) {
+ for (unsigned int n = 0; n < RD->getNumBases(); n++) {
This function isn't nearly correct. IT is only testing the first base (over
and over, but returns immediately...).
Comment at: lib/Sema/SemaDeclCXX.cpp:2396
+
+return (Base && Base->getName() == "IUnknown" &&
+!strcmp(Base->getAttr()->getSpelling(),"uuid"));
Note that this can go down a number of layers. You have to ensure that it goes
multiple layers down. You may wish to make this recursive, and to use your
"IsUnknownType" function..
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: Interface class with uuid base record
zahiraam updated this revision to Diff 114201.
zahiraam added a comment.
Just upload a new patch. Please review.
https://reviews.llvm.org/D37308
Files:
lib/Sema/SemaDeclCXX.cpp
test/SemaCXX/ms-uuid.cpp
Index: test/SemaCXX/ms-uuid.cpp
===
--- test/SemaCXX/ms-uuid.cpp
+++ test/SemaCXX/ms-uuid.cpp
@@ -93,3 +93,28 @@
[uuid("00A0---C000-0049"),
uuid("00A0---C000-0049")] class C10;
}
+
+struct __declspec(uuid("---C000-0046")) IUnknown {};
+__interface ISfFileIOPropertyPage : public IUnknown {};
+
+struct __declspec(uuid("---C000-0045")) IUnknown1 {};
+__interface ISfFileIOPropertyPage1 : public IUnknown1 {}; // expected-error
{{interface type cannot inherit from}}
+
+struct __declspec(uuid("---C000-0046")) IUnknown2 {};
+struct IPropertyPage2 : public IUnknown2 {};
+__interface ISfFileIOPropertyPage2 : public IPropertyPage2 {}; //
expected-error {{interface type cannot inherit from}}
+
+struct IPropertyPage3 : public IUnknown {};
+__interface ISfFileIOPropertyPage3 : public IPropertyPage3 {};
+
+
+__interface __declspec(dllimport) ISfFileIOPropertyPage33 : public IUnknown {};
+
+struct __declspec(uuid("---C000-0046")) IUnknown4 {};
+__interface ISfFileIOPropertyPage4 : public IUnknown4 {}; // expected-error
{{interface type cannot inherit from}}
+
+class __declspec(uuid("---C000-0046")) IUnknown5 {};
+__interface ISfFileIOPropertyPage5 : public IUnknown5 {}; // expected-error
{{interface type cannot inherit from}}
+
+struct __declspec(dllexport) IUnknown6{};
+__interface foo : public IUnknown6{}; // expected-error {{interface type
cannot inherit from}}
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -2373,6 +2373,32 @@
return true;
}
+/// \brief Tests if the __interface base is public.
+static bool IsBasePublicInterface(const CXXRecordDecl *RD,
+ AccessSpecifier spec) {
+ return RD->isInterface() && spec == AS_public;
+}
+
+/// \brief Test is base RD is an uuid Unknown type.
+static bool IsUnknownType(const CXXRecordDecl *RD) {
+ return RD->isStruct() && RD->hasAttr() &&
+ RD->getName() == "IUnknown" &&
+ (RD->getAttr())->getGuid() ==
+ "---C000-0046" &&
+ RD->isEmpty();
+}
+
+static bool IsInheritatedBaseUuid(const CXXRecordDecl *RD) {
+ for (unsigned int n = 0; n < RD->getNumBases(); n++) {
+const CXXRecordDecl *Base =
+RD->bases_begin()->getType()->getAsCXXRecordDecl();
+
+return (Base && Base->getName() == "IUnknown" &&
+!strcmp(Base->getAttr()->getSpelling(),"uuid"));
+ }
+ return false;
+}
+
/// Use small set to collect indirect bases. As this is only used
/// locally, there's no need to abstract the small size parameter.
typedef llvm::SmallPtrSet IndirectBaseSet;
@@ -2450,8 +2476,8 @@
if (const RecordType *Record = NewBaseType->getAs()) {
const CXXRecordDecl *RD = cast(Record->getDecl());
if (Class->isInterface() &&
- (!RD->isInterface() ||
- KnownBase->getAccessSpecifier() != AS_public)) {
+!IsBasePublicInterface(RD, KnownBase->getAccessSpecifier()) &&
+!IsUnknownType(RD) && !IsInheritatedBaseUuid(RD)) {
// The Microsoft extension __interface does not permit bases that
// are not themselves public interfaces.
Diag(KnownBase->getLocStart(), diag::err_invalid_base_in_interface)
Index: test/SemaCXX/ms-uuid.cpp
===
--- test/SemaCXX/ms-uuid.cpp
+++ test/SemaCXX/ms-uuid.cpp
@@ -93,3 +93,28 @@
[uuid("00A0---C000-0049"),
uuid("00A0---C000-0049")] class C10;
}
+
+struct __declspec(uuid("---C000-0046")) IUnknown {};
+__interface ISfFileIOPropertyPage : public IUnknown {};
+
+struct __declspec(uuid("---C000-0045")) IUnknown1 {};
+__interface ISfFileIOPropertyPage1 : public IUnknown1 {}; // expected-error {{interface type cannot inherit from}}
+
+struct __declspec(uuid("---C000-0046")) IUnknown2 {};
+struct IPropertyPage2 : public IUnknown2 {};
+__interface ISfFileIOPropertyPage2 : public IPropertyPage2 {}; // expected-error {{interface type cannot inherit from}}
+
+struct IPropertyPage3 : public IUnknown {};
+__interface ISfFileIOPropertyPage3 : public IPropertyPage3 {};
+
+
+__interface __declspec(dllimport) ISfFileIOPropertyPage33 : public IUnknown {};
+
+struct __declspec(uuid("---C000-0046")) IUnknown4 {};
+__interface ISfFileIOPropertyPage4 : public IUnknown4 {}; // expected-error {{interface type
[PATCH] D37308: Interface class with uuid base record
erichkeane added a comment.
In https://reviews.llvm.org/D37308#858309, @zahiraam wrote:
> Aaron,
>
> Yes I want to this to succeed:
>
> struct __declspec(uuid("---C000-0046")) IUnknown {};
> __interface ISfFileIOPropertyPage : public IUnknown {};
>
> But I also want this to succeed:
>
> struct __declspec(uuid("---C000-0046")) IUnknown {};
> struct IPropertyPage : public IUnknown {};
> __interface ISfFileIOPropertyPage : public IPropertyPage {};
>
> And I can't figure out how these 2 can be differentiated. I think the
> conditions that I have currently are differently too. This later case doesn't
> succeed with the current code.
> And I want this to fail:
>
> class __declspec(uuid("---C000-0046")) IUnknown1 {};
> __interface __declspec(dllimport) ISfFileIOPropertyPage1 : public IUnknown1
> {};
>
> This currently does with the current code.
>
> I guess I need to work on it a bit more.
It definitely looks like you'll have to walk the inheritance tree to get the
2nd example to work. Based on that, the rule seems to be, an __interface can
inherit from:
1 - Another __interface
2 - The COMPLETE IUnknown type, such that it is a struct, with a uuid of
000...C000...46, named "IUnknown", with an empty definition. (note that this is
independent of namespace)
3- Any other type that:
-a: is a class or struct
-b: has no members
-c: inherits from at least 1 type that:
--I: Is an IUnknown type (in any namespace) --or--
--II: A type that satisfies this point 3.
-d: Inherits from no types other than those in -c.
Note that multiple inheritance is permitted:
namespace S{
struct __declspec(uuid("---C000-0046")) IUnknown {};
}
namespace S2{
struct __declspec(uuid("---C000-0046")) IUnknown {};
}
class __declspec(deprecated("asdf")) IPropertyPage2 : S::IUnknown,
S2::IUnknown { }; // Inherits from 2 different IUnknowns
class __declspec(deprecated("asdf")) IPropertyPage3 : S::IUnknown,
S2::IUnknown { };
class IP3 : IPropertyPage2, IPropertyPage3 {}; // Inherits from 2 different
classes that meet #3 above.
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: Interface class with uuid base record
zahiraam added a comment.
Aaron,
Yes I want to this to succeed:
struct __declspec(uuid("---C000-0046")) IUnknown {};
__interface ISfFileIOPropertyPage : public IUnknown {};
But I also want this to succeed:
struct __declspec(uuid("---C000-0046")) IUnknown {};
struct IPropertyPage : public IUnknown {};
__interface ISfFileIOPropertyPage : public IPropertyPage {};
And I can't figure out how these 2 can be differentiated. I think the
conditions that I have currently are differently too. This later case doesn't
succeed with the current code.
And I want this to fail:
class __declspec(uuid("---C000-0046")) IUnknown1 {};
__interface __declspec(dllimport) ISfFileIOPropertyPage1 : public IUnknown1 {};
This currently does with the current code.
I guess I need to work on it a bit more.
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: Interface class with uuid base record
erichkeane requested changes to this revision.
erichkeane added a comment.
This revision now requires changes to proceed.
When forming 'diffs', please make sure you do so against the current status of
Clang. Your latest diff seems to be only against your first commit, so the
diff is crazy looking, and also not submit-able.
@aaron.ballman : The purpose of this patch is to allow this to succeed (Zahira,
please correct me if I'm wrong):
struct __declspec(uuid("---C000-0046")) IUnknown {};
__interface ISfFileIOPropertyPage : public IUnknown {};
Previously, clang's implementation only permitted __interface to inherit from
public __interfaces. You can see these examples in
test/SemaCXX/ms-interface.cpp.
However, our testers discovered that there is an exception to this case: When
the base struct/class has attribute "uuid" defined. Apparently while
researching, @zahiraam also discovered that EVEN IF the inherited struct/class
has UUID, any attribute on the __interface makes said inheritance an error.
THAT SAID, in researching this to try to explain this better, I just discovered
that this diagnosis is actually not correct. It seems that CL has some
'special cases' for the inheritence from a struct/class. For example:
struct __declspec(uuid("---C000-0046")) IUnknown {};
__interface ISfFileIOPropertyPage : public IUnknown {};
Correctly compiles in CL.
class __declspec(uuid("---C000-0046")) IUnknown {};
__interface ISfFileIOPropertyPage : public IUnknown {};
DOES NOT, with the only difference being struct->class.
Additionally,
struct __declspec(uuid("---C000-0046")) IUnknown2 {};
__interface ISfFileIOPropertyPage : public IUnknown2 {};
struct __declspec(uuid("---C000-0045")) IUnknown {};
__interface ISfFileIOPropertyPage : public IUnknown {};
Fails in CL, with the only difference being changing the UUID by 1 character.
On the other hand,
struct __declspec(uuid("---C000-0046")) IUnknown {};
__interface ISfFileIOPropertyPage : IUnknown {};
DOES compile in CL (note the lack of 'public' inheritence, 'private' there also
permits this to compile.
Additionally if IUnknown is not empty (i tried adding 'int x;' to it), this
ALSO fails in CL.
Fails to compile in CL. Note that the ONLY change I made was to rename the
base-struct. I believe that this commit is being way too liberal in this case.
A solution to this SHOULD happen, since this extensively affects the Windows
SDK, however @zahiraam likely needs to figure out what the ACTUAL exceptions in
CL are. I suspect strongly that the exception here is going to be:
-Base is struct
-base has the UUID (listed above)
-Base is named "IUnknown"
-Base is empty.
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: Interface class with uuid base record
zahiraam added a comment. That's both of the code snip-its are supposed to fail (take the diagnostic). If I remove the ClasshasAttrs() condition, from line #2459, these snip-its will pass. I was trying to explain the need for that part of the condition. 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: Interface class with uuid base record
aaron.ballman added a comment. In https://reviews.llvm.org/D37308#858035, @zahiraam wrote: > The test case you gave doesn't compile. It returns the diagnostic. CL has the > same behavior. I don't think it is because of the dllimport. My test case was based off your example code, not something I had actually tried. Your original example doesn't compile with CL either, even when you remove the dllimport. 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: Interface class with uuid base record
zahiraam added a comment. The test case you gave doesn't compile. It returns the diagnostic. CL has the same behavior. I don't think it is because of the dllimport. 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: Interface class with uuid base record
aaron.ballman added a comment.
In https://reviews.llvm.org/D37308#857607, @zahiraam wrote:
> Removed the helper function.
>
> If RD (base class) has uuid attribute, we want to ensure that the interface
> doesn't have attributes. Otherwise cases like:
>
> class __declspec(uuid("---C000-0046")) IUnknown1 {};
> __interface __declspec(dllimport) ISfFileIOPropertyPage1 : public IUnknown1
> {};
>
> will compile.
Is there a reason this code shouldn't work?
class __declspec(uuid("---C000-0046")) IUnknown1 {};
__interface __declspec(deprecated("Don't use this")) blah : public IUnknown1
{};
Is it only dllimport that is a problem?
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: Interface class with uuid base record
zahiraam updated this revision to Diff 113387.
zahiraam added a comment.
Removed the helper function.
If RD (base class) has uuid attribute, we want to ensure that the interface
doesn't have attributes. Otherwise cases like:
class __declspec(uuid("---C000-0046")) IUnknown1 {};
__interface __declspec(dllimport) ISfFileIOPropertyPage1 : public IUnknown1 {};
will compile.
https://reviews.llvm.org/D37308
Files:
lib/Sema/SemaDeclCXX.cpp
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -2398,13 +2398,6 @@
}
}
-
-/// \brief Tests if the __interface base is public.
-static bool IsBasePublicInterface(const CXXRecordDecl *RD,
- AccessSpecifier spec) {
- return RD->isInterface() && spec == AS_public;
-}
-
/// \brief Performs the actual work of attaching the given base class
/// specifiers to a C++ class.
bool Sema::AttachBaseSpecifiers(CXXRecordDecl *Class,
@@ -2457,13 +2450,13 @@
if (const RecordType *Record = NewBaseType->getAs()) {
const CXXRecordDecl *RD = cast(Record->getDecl());
if (Class->isInterface() &&
-!IsBasePublicInterface(RD, KnownBase->getAccessSpecifier()) &&
+!(RD->isInterface() &&
+ KnownBase->getAccessSpecifier() == AS_public) &&
// The Microsoft extension __interface does not permit bases that
// are not themselves public interfaces.
// An interface can inherit from a base, as long as it has
// uuid attributes.
-(!RD->getAttr() ||
-Class->hasAttrs())) {
+(!RD->getAttr() || Class->hasAttrs())) {
Diag(KnownBase->getLocStart(), diag::err_invalid_base_in_interface)
<< getRecordDiagFromTagKind(RD->getTagKind()) << RD->getName()
<< RD->getSourceRange();
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -2398,13 +2398,6 @@
}
}
-
-/// \brief Tests if the __interface base is public.
-static bool IsBasePublicInterface(const CXXRecordDecl *RD,
- AccessSpecifier spec) {
- return RD->isInterface() && spec == AS_public;
-}
-
/// \brief Performs the actual work of attaching the given base class
/// specifiers to a C++ class.
bool Sema::AttachBaseSpecifiers(CXXRecordDecl *Class,
@@ -2457,13 +2450,13 @@
if (const RecordType *Record = NewBaseType->getAs()) {
const CXXRecordDecl *RD = cast(Record->getDecl());
if (Class->isInterface() &&
-!IsBasePublicInterface(RD, KnownBase->getAccessSpecifier()) &&
+!(RD->isInterface() &&
+ KnownBase->getAccessSpecifier() == AS_public) &&
// The Microsoft extension __interface does not permit bases that
// are not themselves public interfaces.
// An interface can inherit from a base, as long as it has
// uuid attributes.
-(!RD->getAttr() ||
- Class->hasAttrs())) {
+(!RD->getAttr() || Class->hasAttrs())) {
Diag(KnownBase->getLocStart(), diag::err_invalid_base_in_interface)
<< getRecordDiagFromTagKind(RD->getTagKind()) << RD->getName()
<< RD->getSourceRange();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37308: Interface class with uuid base record
aaron.ballman added inline comments.
Comment at: lib/Sema/SemaDeclCXX.cpp:2403-2406
+static bool IsBasePublicInterface(const CXXRecordDecl *RD,
+ AccessSpecifier spec) {
+ return RD->isInterface() && spec == AS_public;
+}
I'm not certain that this helper function helps all that much.
Comment at: lib/Sema/SemaDeclCXX.cpp:2465-2466
+// uuid attributes.
+(!RD->getAttr() ||
+Class->hasAttrs())) {
Diag(KnownBase->getLocStart(), diag::err_invalid_base_in_interface)
This should be using `RD->hasAttr()` since you don't need to use the
resulting AST information. Also, why are you checking `Class->hasAttrs()`?
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: Interface class with uuid base record
erichkeane added a comment. 1- You need to do your diff with -U9 so that we get the full context of the file. 2- Better explain the issue in the description 3 SemaDeclCXX.cpp changes look like they need clang-format run on them. 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: Interface class with uuid base record
zahiraam created this revision.
Added support for interface inheriting from a uuid base record.
https://reviews.llvm.org/D37308
Files:
lib/Sema/SemaDeclCXX.cpp
test/SemaCXX/ms-uuid.cpp
Index: test/SemaCXX/ms-uuid.cpp
===
--- test/SemaCXX/ms-uuid.cpp
+++ test/SemaCXX/ms-uuid.cpp
@@ -93,3 +93,15 @@
[uuid("00A0---C000-0049"),
uuid("00A0---C000-0049")] class C10;
}
+
+struct __declspec(uuid("---C000-0046")) IUnknown {};
+__interface ISfFileIOPropertyPage : public IUnknown {};
+
+class __declspec(uuid("---C000-0046")) IUnknown1 {};
+__interface __declspec(dllimport) ISfFileIOPropertyPage1 : public IUnknown1
{}; // expected-error{{interface type cannot inherit from}}
+
+struct __declspec(uuid("---C000-0046")) IUnknown2 {};
+__interface __declspec(dllimport) ISfFileIOPropertyPage2 : public IUnknown2
{}; // expected-error{{interface type cannot inherit from}}
+
+struct __declspec(dllexport) IUnknown3{};
+__interface foo : public IUnknown3{}; // expected-error{{interface type cannot
inherit from}}
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -2398,6 +2398,13 @@
}
}
+
+/// \brief Tests if the __interface base is public.
+static bool IsBasePublicInterface(const CXXRecordDecl *RD,
+ AccessSpecifier spec) {
+ return RD->isInterface() && spec == AS_public;
+}
+
/// \brief Performs the actual work of attaching the given base class
/// specifiers to a C++ class.
bool Sema::AttachBaseSpecifiers(CXXRecordDecl *Class,
@@ -2450,10 +2457,13 @@
if (const RecordType *Record = NewBaseType->getAs()) {
const CXXRecordDecl *RD = cast(Record->getDecl());
if (Class->isInterface() &&
- (!RD->isInterface() ||
- KnownBase->getAccessSpecifier() != AS_public)) {
- // The Microsoft extension __interface does not permit bases that
- // are not themselves public interfaces.
+!IsBasePublicInterface(RD, KnownBase->getAccessSpecifier()) &&
+// The Microsoft extension __interface does not permit bases that
+// are not themselves public interfaces.
+// An interface can inherit from a base, as long as it has
+// uuid attributes.
+(!RD->getAttr() ||
+Class->hasAttrs())) {
Diag(KnownBase->getLocStart(), diag::err_invalid_base_in_interface)
<< getRecordDiagFromTagKind(RD->getTagKind()) << RD->getName()
<< RD->getSourceRange();
Index: test/SemaCXX/ms-uuid.cpp
===
--- test/SemaCXX/ms-uuid.cpp
+++ test/SemaCXX/ms-uuid.cpp
@@ -93,3 +93,15 @@
[uuid("00A0---C000-0049"),
uuid("00A0---C000-0049")] class C10;
}
+
+struct __declspec(uuid("---C000-0046")) IUnknown {};
+__interface ISfFileIOPropertyPage : public IUnknown {};
+
+class __declspec(uuid("---C000-0046")) IUnknown1 {};
+__interface __declspec(dllimport) ISfFileIOPropertyPage1 : public IUnknown1 {}; // expected-error{{interface type cannot inherit from}}
+
+struct __declspec(uuid("---C000-0046")) IUnknown2 {};
+__interface __declspec(dllimport) ISfFileIOPropertyPage2 : public IUnknown2 {}; // expected-error{{interface type cannot inherit from}}
+
+struct __declspec(dllexport) IUnknown3{};
+__interface foo : public IUnknown3{}; // expected-error{{interface type cannot inherit from}}
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -2398,6 +2398,13 @@
}
}
+
+/// \brief Tests if the __interface base is public.
+static bool IsBasePublicInterface(const CXXRecordDecl *RD,
+ AccessSpecifier spec) {
+ return RD->isInterface() && spec == AS_public;
+}
+
/// \brief Performs the actual work of attaching the given base class
/// specifiers to a C++ class.
bool Sema::AttachBaseSpecifiers(CXXRecordDecl *Class,
@@ -2450,10 +2457,13 @@
if (const RecordType *Record = NewBaseType->getAs()) {
const CXXRecordDecl *RD = cast(Record->getDecl());
if (Class->isInterface() &&
- (!RD->isInterface() ||
- KnownBase->getAccessSpecifier() != AS_public)) {
- // The Microsoft extension __interface does not permit bases that
- // are not themselves public interfaces.
+!IsBasePublicInterface(RD, KnownBase->getAccessSpecifier()) &&
+// The Microsoft extension __interface does not permit bases that
+// are not themselves public interfaces.
+// An interface
