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 (
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
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 ha
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
___
c
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 does
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,
+ A
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/S
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
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
+!IsDecl
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
+
+ re
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
=
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.
==
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_ca
erichkeane added a comment.
By my reading of the code, you still haven't fixed this example:
struct __declspec(uuid("---C000-0046")) IUnknown {};
struct IPropertyPageBase : public IUnknown {};
struct IPropertyPage : public IPropertyPageBase {};
__interface foo {
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
==
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
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
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
===
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
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 i
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
===
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
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"))
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 looki
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
_
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
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
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("--
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")) IUnknow
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
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
_
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
===
---
32 matches
Mail list logo
