[cfe-commits] [Patch] Fix for r163013 regression and further __interface enhancement

[Richard Smith]

On Wed, Sep 12, 2012 at 2:08 AM, Andy Gibbs <andyg1001 at hotmail.co.uk> wrote:

> Hi,
>
> Attached are two patches to complement the recent work done by David
> Robins on __interface.
>
> The first, interface-fix.diff, corrects a codegen regression created by
> r163013 whereby all methods (including constructors) inside an __interface
> were being marked pure virtual and were causing the vtable generation to be
> invalid.
>

Applying this to operators doesn't seem correct, since an __interface
shouldn't have any operators in the first place.  Also,
getOverloadedOperator doesn't check for conversion operators.

+    // If this is a method defined in an __interface, and is not a
constructor
+    // or an overloaded operator, then set the pure flag (isVirtual will
already
+    // return true).
+    if (const CXXRecordDecl *Parent =
+          dyn_cast<CXXRecordDecl>(NewFD->getDeclContext())) {
+      if (Parent->isInterface()
+            && !isa<CXXConstructorDecl>(NewFD)
+            && !isa<CXXDestructorDecl>(NewFD)
+            && NewFD->getOverloadedOperator() == OO_None) {

Since an __interface can't contain a declaration of a constructor or a
destructor, how about just "if (Parent->isInterface() &&
NewFD->isUserDeclared())" in both places? (I don't like that we compute
this for isVirtual on demand but set isPure here, but that's a pre-existing
oddity.)

The second, interface-enh.diff, adds in diagnostics for __interface, to
> match the spec laid down by Microsoft, i.e. that interfaces:
>
> * Can inherit from zero or more base interfaces.
> * Cannot inherit from a base class.
> * Can only contain public, pure virtual methods.
> * Cannot contain constructors, destructors, or operators.
> * Cannot contain static methods.
> * Cannot contain data members; properties are allowed.
>
> [source: http://msdn.microsoft.com/en-**us/library/50h7kwtb(v=vs.110).**
> aspx <http://msdn.microsoft.com/en-us/library/50h7kwtb(v=vs.110).aspx>]
>
> And another not explicitly in the spec:
>
> * Cannot be declared 'final'
>

 Sema::ActOnCXXMemberDeclarator(Scope *S, AccessSpecifier AS, Declarator &D,
                                MultiTemplateParamsArg
TemplateParameterLists,
                                Expr *BW, const VirtSpecifiers &VS,
-                               InClassInitStyle InitStyle) {
+                               InClassInitStyle InitStyle, bool
IsInterface) {

You don't need to pass this into Sema; it can work it out from CurContext.
The other checks you added to Parse seem like they would fit better as Sema
checks too.


+  // expected-error at +1 {{user-declared constructor 'I1' is not permitted
within an interface type}}
+  I1();
+  // expected-error at +1 {{user-declared destructor '~I1' is not permitted
within an interface type}}
+  ~I1();

It seems unnecessary to provide the names of the ctor and dtor in these
diagnostics. They don't add anything.


This diagnostic is undesirable:

+__interface I1 {
[...]
+  // expected-warning at +1 {{function definition with pure-specifier is a
Microsoft extension}}
+  void fn2() {

and this one:

+// expected-warning at +1 {{function definition with pure-specifier is a
Microsoft extension}}
+void I1::fn1() const { // It is permissible to give function bodies to
interface methods
+}                      // under the Microsoft extension noted above.

The diagnostic is about "void f() = 0 { }" not being accepted by the
grammar, so doesn't apply here.


+private:
+  // expected-error at +1 {{non-public member function 'fn2' is not permitted
within an interface type}}
+  void fn2();

It's a bit surprising to me that the access-specifier is allowed here at
all, but if this matches MSVC then fair enough.

Thanks!
Richard
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120912/0156018f/attachment.html>