[PATCH] D37308: Interface class with uuid base record

[Erich Keane via Phabricator via cfe-commits]

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<UuidAttr>();
----------------
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("00000000-0000-0000-C000-000000000046")) 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