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

[Erik Verbruggen]

On 27 Sep 2011, at 23:27, Douglas Gregor wrote:

> A few comments on the patch itself:
> 
> diff --git a/include/clang/Basic/Attr.td b/include/clang/Basic/Attr.td
> index 4d9aa5b..e6f4119 100644
> --- a/include/clang/Basic/Attr.td
> +++ b/include/clang/Basic/Attr.td
> @@ -647,3 +647,15 @@ def SharedLocksRequired : InheritableAttr {
>   let Args = [VariadicExprArgument<"Args">];
>   let LateParsed = 1;
> }
> +
> +def QInvokable : InheritableAttr {
> +  let Spellings = ["q_invokable"];
> +}
> +
> +def QSignal : InheritableAttr {
> +  let Spellings = ["q_signal"];
> +}
> +
> +def QSlot : InheritableAttr {
> +  let Spellings = ["q_slot"];
> +}
> 
> Why the "q_" prefer rather than "qt_"? Is it because of the Q_-prefixed macros? I ask because "qt_" seems more descriptive.

Yes, Qt uses Q_blah macros. Then again, Clang is not Qt, and I do not have a strong opinion on this. Should I change the attribute names, and if so, should I also change the enumerators in CXCursorKind to read CXCursor_QtSignalAttr, etc?

> diff --git a/include/clang/Parse/Parser.h b/include/clang/Parse/Parser.h
> index d32c36e..85ad602 100644
> --- a/include/clang/Parse/Parser.h
> +++ b/include/clang/Parse/Parser.h
> @@ -1047,7 +1047,8 @@ private:
>   void DeallocateParsedClasses(ParsingClass *Class);
>   void PopParsingClass(Sema::ParsingClassState);
> 
> -  Decl *ParseCXXInlineMethodDef(AccessSpecifier AS, ParsingDeclarator &D,
> +  Decl *ParseCXXInlineMethodDef(AccessSpecifier AS, AttributeList *QtAttrs,
> +                                ParsingDeclarator &D,
>                                 const ParsedTemplateInfo &TemplateInfo,
>                                 const VirtSpecifiers& VS, ExprResult& Init);
> 
> I'd rather call this "AccessAttrs" rather than "QtAttrs". Open up a new place for people to add attributes, and they'll find new (non-Qt-related) reasons to add attributes there :)

Ok.

> @@ -2131,10 +2138,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
> +        
> +        xsAttrs.clear();
> +        MaybeParseGNUAttributes(xsAttrs);
> +        
> +        if (Tok.is(tok::colon)) {
> +          Decl *xsDecl = Actions.ActOnAccessSpecifier(AS, ASLoc,
> +                                                      Tok.getLocation());
> +          if (AttributeList *Attrs = xsAttrs.getList())
> +            Actions.ProcessDeclAttributeList(Actions.CurScope, xsDecl, Attrs, 
> +                                             false, true);
> +        } else {
>           Diag(Tok, diag::err_expected_colon);
> +        }
> 
> Could we do some more validation here, so that we reject all attributes *except* those explicitly marked as being okay for access specifiers?

Sure.

Thanks, I will create a new patch including this feedback.

-- Erik.