[cfe-dev] Proposal: parsing Qt signals/slots with attributes

[Douglas Gregor]

On Oct 9, 2011, at 8:35 AM, Erik Verbruggen wrote:

> On 07 Oct, 2011,at 10:32 PM, Douglas Gregor <dgregor at apple.com> wrote:
> 
>> 
>> On Oct 6, 2011, at 2:12 AM, Erik Verbruggen wrote:
>> 
>> > On 30 Sep, 2011,at 07:34 AM, Douglas Gregor <dgregor at apple.com> wrote:
>> > 
>> >> Let's use the existing "annotate" attribute, which is a bit like the "user" attribute mentioned in (2) above, but already implemented in Clang. That way, Qt can have definitions such as:
>> >> 
>> >> #define Q_OBJECT __attribute__((annotate("qt_object")))
>> >> 
>> >> Then, you patch need only:
>> >> 
>> >> (a) add support for parsing attributes on access specifiers, and
>> >> (b) update libclang to expose the "annotate" attribute 
>> >> 
>> >> That should be sufficient, requires much less effort (and risk) than most of the other solutions, and won't require us to bring anything related to Qt into Clang itself.
>> > 
>> > Attached is a new patch, which adds parsing of attributes after a C++ access specifier. These attributes get propagated in the same manner as the access specifier.
>> 
>> I see that this handles, e.g.,
>> 
>> public slots:
>> 
>> (with the attribute definition of "slots", of course)
>> 
>> but it doesn't seem to handle, e.g.,
>> 
>> signals:
>> 
>> I assume you want to address that as well?
>  
> qobjectdefs.h uses:
> #   define slots
> #   define signals protected
> 
> so with attributes one can use:
> #   define slots __attribute__((annotate("qt_slot")))
> #   define signals protected __attribute__((annotate("qt_signal")))

Ah, okay. Thanks for the clarification.

>> 
>> > Then in c-index-test I changed the spelling of the annotate attribute to return the quoted string.
>> 
>> That makes sense, thanks.
>> 
>> > Question: should I add a check to disallow attributes other than the annotate attribute after an access specifier?
>> 
>> Yes. I'd like us to enable such attributes on a case-by-case basis, rather than deal with the fallout of them all implicitly propagating.
>  
> Ok, added it.

I assume that this was meant to catch it:

--- a/lib/Sema/SemaDeclAttr.cpp
+++ b/lib/Sema/SemaDeclAttr.cpp
@@ -3482,6 +3482,13 @@ static void ProcessNonInheritableDeclAttr(Sema &S, Scope *scope, Decl *D,
 
 static void ProcessInheritableDeclAttr(Sema &S, Scope *scope, Decl *D,
                                        const AttributeList &Attr) {
+  if (isa<AccessSpecDecl>(D) && Attr.getKind() == AttributeList::AT_annotate) {
+    // Annotation attributes are the only attributes allowed after an access
+    // specifier.
+    S.Diag(Attr.getLoc(), diag::err_only_annotate_after_access_spec);
+    return;
+  }
+
   switch (Attr.getKind()) {
   case AttributeList::AT_IBAction:            handleIBAction(S, D, Attr); break;
     case AttributeList::AT_IBOutlet:          handleIBOutlet(S, D, Attr); break;

but the condition is wrong (it errors when we have an access declaration with an "annotate" attribute), and it's clearly not working:

struct X {
  public __attribute__((unavailable)):
  void foo();
};

void f(X x) {
  x.foo();
}

here, there's no diagnostic complaining about the "unavailable" attribute on the access specifier, and the "unavailable" attribute gets propagated down to "foo()". Most likely, you want to pass the access specifier attributes through the call to ActOnAccessSpecifier, here:

@@ -2139,10 +2146,19 @@ void Parser::ParseCXXMemberSpecification(SourceLocation RecordLoc,
         CurAS = AS;
         SourceLocation ASLoc = Tok.getLocation();
         ConsumeToken();
-        if (Tok.is(tok::colon))
-          Actions.ActOnAccessSpecifier(AS, ASLoc, Tok.getLocation());
-        else
+
+        AccessAttrs.clear();
+        MaybeParseGNUAttributes(AccessAttrs);
+
+        if (Tok.is(tok::colon)) {
+          Decl *AccessDecl = Actions.ActOnAccessSpecifier(AS, ASLoc,
+                                                          Tok.getLocation());
+          if (AttributeList *Attrs = AccessAttrs.getList())
+            Actions.ProcessDeclAttributeList(Actions.CurScope, AccessDecl,
+                                             Attrs, false, true);
+        } else {
           Diag(Tok, diag::err_expected_colon);
+        }
         ConsumeToken();
         continue;
       }

so the attributes will get tacked on to the AccessSpecDecl (for good source fidelity) and you'll have a chance to diagnostic non-annotate attributes.

	- Doug

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20111009/ed8187c3/attachment.html>