interface objects in MSVC are permitted to inherit from interface types, and interface-like types.
Additionally, there are two default interface-like types (IUnknown and IDispatch) that all interface-like
types must inherit from.
Differential D37308
Fix the __interface inheritence rules to work better with IUnknown and IDispatch erichkeane on Aug 30 2017, 12:51 PM. Authored by
Details
interface objects in MSVC are permitted to inherit from interface types, and interface-like types. Additionally, there are two default interface-like types (IUnknown and IDispatch) that all interface-like
Diff Detail
Event TimelineComment Actions 1- You need to do your diff with -U99999 so that we get the full context of the file.
Comment Actions 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("00000000-0000-0000-C000-000000000046")) IUnknown1 {}; will compile. Comment Actions Is there a reason this code shouldn't work? class __declspec(uuid("00000000-0000-0000-C000-000000000046")) IUnknown1 {}; __interface __declspec(deprecated("Don't use this")) blah : public IUnknown1 {}; Is it only dllimport that is a problem? Comment Actions 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. Comment Actions 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. Comment Actions 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. Comment Actions 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("00000000-0000-0000-C000-000000000046")) 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("00000000-0000-0000-C000-000000000046")) IUnknown {}; __interface ISfFileIOPropertyPage : public IUnknown {}; Correctly compiles in CL. class __declspec(uuid("00000000-0000-0000-C000-000000000046")) IUnknown {}; __interface ISfFileIOPropertyPage : public IUnknown {}; DOES NOT, with the only difference being struct->class. Additionally, struct __declspec(uuid("00000000-0000-0000-C000-000000000046")) IUnknown2 {}; __interface ISfFileIOPropertyPage : public IUnknown2 {}; struct __declspec(uuid("00000000-0000-0000-C000-000000000045")) 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("00000000-0000-0000-C000-000000000046")) 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: Comment Actions Aaron, Yes I want to this to succeed: struct declspec(uuid("00000000-0000-0000-C000-000000000046")) IUnknown {}; But I also want this to succeed: struct declspec(uuid("00000000-0000-0000-C000-000000000046")) IUnknown {}; 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. class declspec(uuid("00000000-0000-0000-C000-000000000046")) IUnknown1 {}; This currently does with the current code. I guess I need to work on it a bit more. Comment Actions 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: Note that multiple inheritance is permitted: namespace S{ struct __declspec(uuid("00000000-0000-0000-C000-000000000046")) IUnknown {}; } namespace S2{ struct __declspec(uuid("00000000-0000-0000-C000-000000000046")) 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.
Comment Actions 1 more thing I missed.
Comment Actions By my reading of the code, you still haven't fixed this example: struct __declspec(uuid("00000000-0000-0000-C000-000000000046")) IUnknown {}; struct IPropertyPageBase : public IUnknown {}; struct IPropertyPage : public IPropertyPageBase {}; __interface foo {}; __interface bar {}; __interface Prop2 : foo, bar, IUnknown{}; __interface Prop3 : foo, Prop2{}; According to godbolt, all compile w/o error.
Comment Actions 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 Actions 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 Actions 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 Actions 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 Actions Erich, The IsOrInheritsFromIUnknown function is needed.
Comment Actions MSVC and xmain compile this: But MSVC doesn't compile this: struct declspec(uuid("00000000-0000-0000-C000-000000000046")) IUnknown {}; So a base of RD that has siblings and inherits from a Unknown is not permitted. Comment Actions 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. Comment Actions 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-0000-0000-c000-000000000046" A type is 'interface like' if: An interface-like 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. Comment Actions SO, the implementation would likely be (~2489): (KnownBase->getAccessSpecifier() != TTK_public || !RD->isInterfaceLike()) { with "isInterfaceLike" testing what I outlined above, including checking 'is interface' first. **Edit wtih a better idea that actually gets parens right Comment Actions @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). Comment Actions 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.
Comment Actions Thanks for the quick response! New patch coming soon.
|