Proposal: parsing Qt signals/slots with attributes

I had completely forgotten about these. How much work is involved in making attribute namespaces work?

  - Doug

Alright, since there haven't been enough crazy ideas today… I think we should consider defining 'signals' and 'slots' as context-sensitive keywords, rather than handling these as attributes on the access specifier. The grammar is fairly simple, and one need only look ahead a single token (to the ':') to determine whether we have the signals or slots keyword (vs. using it as an identifier). It shouldn't be much more work than threading attributes through the access specifiers like you've already done.

IMO, the end result would be much cleaner than having to #define signals and slots (#define'ing Q_SIGNAL, Q_SLOT, etc. is fine by me), and has the added benefit of helping resolve my longstanding grudge with Qt :slight_smile:

  http://www.boost.org/doc/libs/1_47_0/doc/html/signals/s04.html#id2861476

  - Doug

It depends on whether we want them to work or work nicely, and whether
or not we want them for GCC attributes. If we just want them to work
and we don't care about GCC syntax, stuffing special-case parsing into
ParseCXX0XAttributes would work fine. If we want them to work nicely,
we would have to put some more work into the TableGen to use the
Namespaces attribute on Attr to generate some code.

Sean

I had completely forgotten about these. How much work is involved in making attribute namespaces work?

       - Doug

It depends on whether we want them to work or work nicely, and whether
or not we want them for GCC attributes. If we just want them to work
and we don't care about GCC syntax,

Aye, there's the rub. We want to stick with GCC attribute syntax for these attributes, since we don't want to require C++0x. I guess we could talk with the GCC people to see if they're planing to pull attribute namespaces into GNU attributes, in which case we could do the same. That would be fine.

stuffing special-case parsing into
ParseCXX0XAttributes would work fine. If we want them to work nicely,
we would have to put some more work into the TableGen to use the
Namespaces attribute on Attr to generate some code.

As much as I'd like to get the general solution, time restrictions may force us into special-casing this.

  - Doug

Out of curiosity, how would this satisfy criteria (4) of the clang language extension guidelines ( http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-July/016275.html ) - would this be practically standardizable? (it certainly fulfills several, perhaps all, of the other criteria - existing community, etc)

Is it possible to support C++11 attributes (& namespaced attributes) as an extension in C++03, or does that break other things? Seems a pity to further extend an extension that’s been superseded by a standard feature.

  • David

28.09.2011, 21:50, "Douglas Gregor":

Alright, since there haven't been enough crazy ideas today… I think we should consider defining 'signals' and 'slots' as context-sensitive keywords, rather than handling these as attributes on the access specifier. The grammar is fairly simple, and one need only look ahead a single token (to the ':') to determine whether we have the signals or slots keyword (vs. using it as an identifier). It shouldn't be much more work than threading attributes through the access specifiers like you've already done.

IMO, the end result would be much cleaner than having to #define signals and slots (#define'ing Q_SIGNAL, Q_SLOT, etc. is fine by me), and has the added benefit of helping resolve my longstanding grudge with Qt :slight_smile:

http://www.boost.org/doc/libs/1_47_0/doc/html/signals/s04.html#id2861476

It seems to me as there is a danger of keyword misinterpretation in non-QObject contexts.

I don't see any ambiguity. Can you provide an example?

  - Doug

Hello,

I would like to add support for recognising Qt’s signal/slot mechanism to clang by using attributes. For that, I have a proposal for a few small changes to the clang parser. These changes use the attributes to mark AST nodes as signal/slot/invokable, and expose those attributes through libclang.

A bit of background: if you are more familiar with Objective-C, then the signal/slot mechanism works nearly exactly the same as Cocoa’s IBAction/IBOutlet: it uses a meta-type system to invoke the methods, or let slots connected to a signal know that the signal was emitted. Qt’s meta-type system also allows for “invokable methods”, which can be called/invoked without being tagged as slot.

Example:

class Test {
public:
Q_INVOKABLE void invokableA();

public slots:
void slotA();
void inlineSlotB() {}

signals:
void signalC();

private:
Q_SIGNAL void signalD(), signalE();
Q_SLOT void slotF(), slotG();
void not_a_slot_or_slot();
Q_SLOT void inlineSlotH() {}
};

(To be used properly with Qt, the class would also have to inherit from QObject, and have the Q_OBJECT macro right after the opening brace.)

Taking a leaf from Cocoa’s book, we can use the pre-processor to inject:

#define signals protected attribute((q_signal))
#define slots attribute((q_slot))

Alright, since there haven’t been enough crazy ideas today… I think we should consider defining ‘signals’ and ‘slots’ as context-sensitive keywords, rather than handling these as attributes on the access specifier. The grammar is fairly simple, and one need only look ahead a single token (to the ‘:’) to determine whether we have the signals or slots keyword (vs. using it as an identifier). It shouldn’t be much more work than threading attributes through the access specifiers like you’ve already done.

Out of curiosity, how would this satisfy criteria (4) of the clang language extension guidelines ( http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-July/016275.html ) - would this be practically standardizable? (it certainly fulfills several, perhaps all, of the other criteria - existing community, etc)

Sadly, it would not satisfy criteria (4), and we would have to live with that, because it’s clearly not something that the committee would ever standardize. The Qt developers of old did us all a grave injustice in the first place by #define’ing lowercased identifiers and then having the gall to become wildly popular, so we’re fairly stuck now: either we do some attribute trickery and keep on defining “signals” and “slots”, or we introduce a not-likely-to-ever-be-standardized feature to make existing Qt code work better and reclaim “signals” and “slots” in the process.

Is it possible to support C++11 attributes (& namespaced attributes) as an extension in C++03, or does that break other things? Seems a pity to further extend an extension that’s been superseded by a standard feature.

We could probably support C++11 attributes in C++03; I don’t think it actually breaks anything.

  • Doug

Since "signals" and "slots" are not language-reserved keywords, there's non-zero probability that some 3rd party can also #define them for some other purposes. So, IMHO it's needed to check if class inherits QObject.

29.09.2011, 04:07, "Douglas Gregor":

Sadly, it would not satisfy criteria (4), and we would have to live with that, because it's clearly not something that the committee would ever standardize.

Surely, but this functionality could happily live in a plugin. Unfortunately, current capabilities of plugins are insufficient to implement needed features, i.e.. context-sensitive keywords or even custom attributes.

Hi all,

So to sum up the options from both the mailing-list and irc, I can go either of these routes:

  1. just go with attributes as with my first patch
  • pro: only a few changes to go, and it is done
  • con: yet another toolkit specific extension which doesn’t help everyone
  1. implement GCC’s user attributes: attribute((user(“your_funky_attribute”)))
  • pro: more GCC compatibility, also useful for projects already using it (i.e. Mozilla)
  • con: would need significantly more work than the other routes
  • con: very GCC and very non standard, possibly obsoleted by:
  1. c++0x attribute namespaces
  • pro: very standard
  • con: support seems to be minimal (if I got it right), so needs some work
  • con (?): c++0x specific, so we would need to support it for other language standards than just c++0x

So, before I spend time implementing all of them, what is the best way (or “the clang way”) to go?

Then independent of that (because it can be exposed/implemented through any of the previous 3 options):

Hi all,

So to sum up the options from both the mailing-list and irc, I can go either of these routes:

  1. just go with attributes as with my first patch
  • pro: only a few changes to go, and it is done
  • con: yet another toolkit specific extension which doesn’t help everyone
  1. implement GCC’s user attributes: attribute((user(“your_funky_attribute”)))
  • pro: more GCC compatibility, also useful for projects already using it (i.e. Mozilla)
  • con: would need significantly more work than the other routes
  • con: very GCC and very non standard, possibly obsoleted by:
  1. c++0x attribute namespaces
  • pro: very standard
  • con: support seems to be minimal (if I got it right), so needs some work
  • con (?): c++0x specific, so we would need to support it for other language standards than just c++0x

So, before I spend time implementing all of them, what is the best way (or “the clang way”) to go?

Then independent of that (because it can be exposed/implemented through any of the previous 3 options):

28.09.2011, 21:50, "Douglas Gregor":

Alright, since there haven't been enough crazy ideas today… I think we should consider defining 'signals' and 'slots' as context-sensitive keywords, rather than handling these as attributes on the access specifier. The grammar is fairly simple, and one need only look ahead a single token (to the ':') to determine whether we have the signals or slots keyword (vs. using it as an identifier). It shouldn't be much more work than threading attributes through the access specifiers like you've already done.

IMO, the end result would be much cleaner than having to #define signals and slots (#define'ing Q_SIGNAL, Q_SLOT, etc. is fine by me), and has the added benefit of helping resolve my longstanding grudge with Qt :slight_smile:

   http://www.boost.org/doc/libs/1_47_0/doc/html/signals/s04.html#id2861476

It seems to me as there is a danger of keyword misinterpretation in non-QObject contexts.

I don't see any ambiguity. Can you provide an example?

Since "signals" and "slots" are not language-reserved keywords, there's non-zero probability that some 3rd party can also #define them for some other purposes.

So? You can break any context-sensitive keyword scheme by #define'ing that identifier as a macro. Any code that today defines signals or slots as a macro in a manner that isn't compatible with Qt's use of these keywords, is already broken when using Qt, so we've lost nothing.

So, IMHO it's needed to check if class inherits QObject.

To direct parsing? We don't want to do that, because we can't know whether this class inherits QObject, and we have to decide at parse time what these keywords mean:

  template<typename T>
  class X : public T {
  slots: // does X inherit QObject? Maybe it does, maybe it doesn't
  };

Whether MOC can actually handle this kind of thing today is, IMO, irrelevant; we shouldn't design the feature in a way that's broken in templates.

  - Doug

29.09.2011, 18:41, "Douglas Gregor":

ššIt seems to me as there is a danger of keyword misinterpretation in non-QObject contexts.

šI don't see any ambiguity. Can you provide an example?

šSince "signals" and "slots" are not language-reserved keywords, there's non-zero probability that some 3rd party can also #define them for some other purposes.

So? You can break any context-sensitive keyword scheme by #define'ing that identifier as a macro.

Ah, that's like #define private public. OK.

Any code that today defines signals or slots as a macro in a manner that isn't compatible with Qt's use of these keywords, is already broken when using Qt, so we've lost nothing.

Actually this code might not be broken:

1. moc doesn't parse all source code of project: with QMake build system, only headers added to "HEADERS" variable and sources containing #include "something.moc" (with other build system behavior may be different). So it's possible to use Qt in one part of project, and use Qt-incompatible code in others, while building everything with one compiler.

2. There is #indef Q_MOC_RUN which can "protect" some parts of code from moc parsing.

šSo, IMHO it's needed to check if class inherits QObject.

To direct parsing? We don't want to do that, because we can't know whether this class inherits QObject, and we have to decide at parse time what these keywords mean:

šššššššštemplate<typename T>
ššššššššclass X : public T {
ššššššššslots: // does X inherit QObject? Maybe it does, maybe it doesn't
šššššššš};

Whether MOC can actually handle this kind of thing today is, IMO, irrelevant; we shouldn't design the feature in a way that's broken in templates.

Thank you for clarification. While it could be useful to prevent code from being incompatible with "traditional" Qt/moc rules (e.g., template cannot contain signals or slots), that could be a task of static analyzer, not regular parser (and we can unleash Qt capabilities if project uses only Clang for compilation).

I should clarify something here, since I did some more research after I mentioned this on IRC. It turns out that what gcc specifically does is allow plugins to register attributes; the dehydra plugin registers a functional attribute called `user'. When this plugin is not enabled for GCC, it still produces legal code, although it produces a warning.

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.

  - Doug

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. Then in c-index-test I changed the spelling of the annotate attribute to return the quoted string.

Question: should I add a check to disallow attributes other than the annotate attribute after an access specifier?

– Erik.

annotate-for-access-specifiers.patch (13.5 KB)

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?

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.

Minor nit:

@@ -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

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”)))

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.

Minor nit:

@@ -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 *xsDecl = Actions.ActOnAccessSpecifier(AS, ASLoc,
  • Tok.getLocation());
  • if (AttributeList *Attrs = AccessAttrs.getList())
  • Actions.ProcessDeclAttributeList(Actions.CurScope, xsDecl, Attrs,
  • false, true);
  • } else {
    Diag(Tok, diag::err_expected_colon);
  • }

http://llvm.org/docs/CodingStandards.html#ll_naming

suggests that “nsDecl” be named starting with a capital letter.

Changed it to AccessDecl.

We also seem to be missing tests.

Should be included now Attached patch is against r141497.

– Erik

0001-libclang-Allow-for-annotate-attributes-after-access-.patch (17.8 KB)

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(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;
  • }