On Wed, Sep 12, 2012 at 2:08 AM, Andy Gibbs <span dir="ltr"><<a href="mailto:andyg1001@hotmail.co.uk" target="_blank">andyg1001@hotmail.co.uk</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi,<br>
<br>
Attached are two patches to complement the recent work done by David Robins on __interface.<br>
<br>
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.<br>
</blockquote><div><br></div><div>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.</div>
<div><br></div><div><div>+ // If this is a method defined in an __interface, and is not a constructor</div><div>+ // or an overloaded operator, then set the pure flag (isVirtual will already</div><div>+ // return true).</div>
<div>+ if (const CXXRecordDecl *Parent =</div><div>+ dyn_cast<CXXRecordDecl>(NewFD->getDeclContext())) {</div><div>+ if (Parent->isInterface()</div><div>+ && !isa<CXXConstructorDecl>(NewFD)</div>
<div>+ && !isa<CXXDestructorDecl>(NewFD)</div><div>+ && NewFD->getOverloadedOperator() == OO_None) {</div></div><div><br></div><div>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.)</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The second, interface-enh.diff, adds in diagnostics for __interface, to match the spec laid down by Microsoft, i.e. that interfaces:<br>
<br>
* Can inherit from zero or more base interfaces.<br>
* Cannot inherit from a base class.<br>
* Can only contain public, pure virtual methods.<br>
* Cannot contain constructors, destructors, or operators.<br>
* Cannot contain static methods.<br>
* Cannot contain data members; properties are allowed.<br>
<br>
[source: <a href="http://msdn.microsoft.com/en-us/library/50h7kwtb(v=vs.110).aspx" target="_blank">http://msdn.microsoft.com/en-<u></u>us/library/50h7kwtb(v=vs.110).<u></u>aspx</a>]<br>
<br>
And another not explicitly in the spec:<br>
<br>
* Cannot be declared 'final'<br></blockquote><div><br></div><div><div> Sema::ActOnCXXMemberDeclarator(Scope *S, AccessSpecifier AS, Declarator &D,</div><div> MultiTemplateParamsArg TemplateParameterLists,</div>
<div> Expr *BW, const VirtSpecifiers &VS,</div><div>- InClassInitStyle InitStyle) {</div><div>+ InClassInitStyle InitStyle, bool IsInterface) {</div>
</div><div><br></div><div>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.</div><div><br></div><div><br>
</div><div><div>+ // expected-error@+1 {{user-declared constructor 'I1' is not permitted within an interface type}}</div><div>+ I1();</div><div>+ // expected-error@+1 {{user-declared destructor '~I1' is not permitted within an interface type}}</div>
<div>+ ~I1();</div></div><div><br></div><div>It seems unnecessary to provide the names of the ctor and dtor in these diagnostics. They don't add anything.</div><div><br></div><div><br></div><div><div>This diagnostic is undesirable:</div>
<div><div><br></div><div>+__interface I1 {</div><div>[...]</div><div><div>+ // expected-warning@+1 {{function definition with pure-specifier is a Microsoft extension}}</div><div>+ void fn2() {</div></div><div><br></div>
</div><div>and this one:</div><div><br></div><div><div>+// expected-warning@+1 {{function definition with pure-specifier is a Microsoft extension}}</div><div>+void I1::fn1() const { // It is permissible to give function bodies to interface methods</div>
<div>+} // under the Microsoft extension noted above.</div></div><div><br></div><div>The diagnostic is about "void f() = 0 { }" not being accepted by the grammar, so doesn't apply here.</div>
</div><div><br></div><div><br></div><div><div>+private:</div><div>+ // expected-error@+1 {{non-public member function 'fn2' is not permitted within an interface type}}</div><div>+ void fn2();</div></div><div><br>
</div><div>It's a bit surprising to me that the access-specifier is allowed here at all, but if this matches MSVC then fair enough.</div><div><br></div><div>Thanks!</div><div>Richard</div></div>