The QtReslot Clang plugin

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

The QtReslot Clang plugin

David Chisnall via cfe-dev
Hello,

During work on an internal Qt-based project at Novasys-Ingenierie,
a Clang plugin was written to convert string-based signals and slots
to the Qt5 syntax. The plugin has since been pushed to Github [1]
in the hope it will be useful to others facing similar issues.

--
Richard Braun

[1] https://github.com/Novasys-Ingenierie/qtreslot
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: The QtReslot Clang plugin

David Chisnall via cfe-dev
On Tuesday, 14 February 2017 20:33:15 CET Richard Braun via cfe-dev wrote:
> Hello,
>
> During work on an internal Qt-based project at Novasys-Ingenierie,
> a Clang plugin was written to convert string-based signals and slots
> to the Qt5 syntax. The plugin has since been pushed to Github [1]
> in the hope it will be useful to others facing similar issues.

Heya Richard,

just for the sake of making you aware of existing tools (to save work & share
ideas): there already is a Qt oriented Clang-based code checker called Clazy
mainly developed by one of my colleagues at KDAB.

Clazy allows all sorts of checking & refactoring in Qt code. It also has a
check for 'old-style-connect' & provides fixits for it: IOW, it will transform
the code to new-style-connect statements if desired.

Worth having a look, maybe you want to contribute some of your code there
instead so it gets available to a wider audience:
  https://github.com/KDE/clazy

Clazy has already been used to refactor *lots* of code in KDE land
automatically.

Hope that helps,
Kevin

PS: CC'ed Sergio, the main author.

--
Kevin Funk | [hidden email] | http://kfunk.org
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

signature.asc (169 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: The QtReslot Clang plugin

David Chisnall via cfe-dev
On 02/14/2017 11:29 PM, Kevin Funk via cfe-dev wrote:

> On Tuesday, 14 February 2017 20:33:15 CET Richard Braun via cfe-dev wrote:
>> Hello,
>>
>> During work on an internal Qt-based project at Novasys-Ingenierie,
>> a Clang plugin was written to convert string-based signals and slots
>> to the Qt5 syntax. The plugin has since been pushed to Github [1]
>> in the hope it will be useful to others facing similar issues.
>
> Heya Richard,
>
> just for the sake of making you aware of existing tools (to save work & share
> ideas): there already is a Qt oriented Clang-based code checker called Clazy
> mainly developed by one of my colleagues at KDAB.
>
> Clazy allows all sorts of checking & refactoring in Qt code. It also has a
> check for 'old-style-connect' & provides fixits for it: IOW, it will transform
> the code to new-style-connect statements if desired.

Hi!

I remember that there was some effort to make this plugin also available
for libclang users. What happened to that?

Nikolai

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: The QtReslot Clang plugin

David Chisnall via cfe-dev
In reply to this post by David Chisnall via cfe-dev
Both of these projects duplicate a lot of code from clang-tidy and the AST matcher infrastructure in clang.

I'd be curious what the reasons are to not use those, so we can try to address them :)

Browsing over the code, I think a large percentage of it could be saved.
For example:
static CXXMethodDecl* isArgMethod(FunctionDecl *func)
{
    if (!func)
        return nullptr;

    CXXMethodDecl *method = dyn_cast<CXXMethodDecl>(func);
    if (!method || method->getNameAsString() != "arg")
        return nullptr;

    CXXRecordDecl *record = method->getParent();
    if (!record || record->getNameAsString() != "QString")
        return nullptr;

    return method;
}

would become the AST matcher:
cxxMethodDecl(hasName("QString::arg"))

Generally, you could find calls to QString::arg with something like:
callExpr(callee(cxxMethodDecl(hasName("QString::arg"))))

Cheers,
/Manuel

On Tue, Feb 14, 2017 at 11:29 PM Kevin Funk via cfe-dev <[hidden email]> wrote:
On Tuesday, 14 February 2017 20:33:15 CET Richard Braun via cfe-dev wrote:
> Hello,
>
> During work on an internal Qt-based project at Novasys-Ingenierie,
> a Clang plugin was written to convert string-based signals and slots
> to the Qt5 syntax. The plugin has since been pushed to Github [1]
> in the hope it will be useful to others facing similar issues.

Heya Richard,

just for the sake of making you aware of existing tools (to save work & share
ideas): there already is a Qt oriented Clang-based code checker called Clazy
mainly developed by one of my colleagues at KDAB.

Clazy allows all sorts of checking & refactoring in Qt code. It also has a
check for 'old-style-connect' & provides fixits for it: IOW, it will transform
the code to new-style-connect statements if desired.

Worth having a look, maybe you want to contribute some of your code there
instead so it gets available to a wider audience:
  https://github.com/KDE/clazy

Clazy has already been used to refactor *lots* of code in KDE land
automatically.

Hope that helps,
Kevin

PS: CC'ed Sergio, the main author.

--
Kevin Funk | [hidden email] | http://kfunk.org_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: The QtReslot Clang plugin

David Chisnall via cfe-dev
In reply to this post by David Chisnall via cfe-dev
On Wednesday, 15 February 2017 09:31:29 CET Nikolai Kosjar via cfe-dev wrote:

> On 02/14/2017 11:29 PM, Kevin Funk via cfe-dev wrote:
> > On Tuesday, 14 February 2017 20:33:15 CET Richard Braun via cfe-dev wrote:
> >> Hello,
> >>
> >> During work on an internal Qt-based project at Novasys-Ingenierie,
> >> a Clang plugin was written to convert string-based signals and slots
> >> to the Qt5 syntax. The plugin has since been pushed to Github [1]
> >> in the hope it will be useful to others facing similar issues.
> >
> > Heya Richard,
> >
> > just for the sake of making you aware of existing tools (to save work &
> > share ideas): there already is a Qt oriented Clang-based code checker
> > called Clazy mainly developed by one of my colleagues at KDAB.
> >
> > Clazy allows all sorts of checking & refactoring in Qt code. It also has a
> > check for 'old-style-connect' & provides fixits for it: IOW, it will
> > transform the code to new-style-connect statements if desired.
>
> Hi!
>
> I remember that there was some effort to make this plugin also available
> for libclang users. What happened to that?
This is in limbo, because for normal release builds my intended patch to allow
loading Clang plugins under libclang does not work.

Clang plugin loading under libclang is a general problem, not related to
Clazy.

See this thread for the discussion:
  http://lists.llvm.org/pipermail/cfe-dev/2016-March/047639.html

And my patch (stalled):
  https://reviews.llvm.org/D15729

Happy to get some help/ideas there so we can finally load e.g. Clazy under
such setups.

Cheers,
Kevin

>
> Nikolai
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


--
Kevin Funk | [hidden email] | http://kfunk.org
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

signature.asc (169 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: The QtReslot Clang plugin

David Chisnall via cfe-dev
In reply to this post by David Chisnall via cfe-dev
On Wed, Feb 15, 2017 at 10:00:25AM +0000, Manuel Klimek wrote:
> Both of these projects duplicate a lot of code from clang-tidy and the AST
> matcher infrastructure in clang.
>
> I'd be curious what the reasons are to not use those, so we can try to
> address them :)

I tried using matchers at some point, but couldn't understand the
processing flow and felt a lot more comfortable with the current result
despite writing more code.

--
Richard Braun
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: The QtReslot Clang plugin

David Chisnall via cfe-dev
On Wed, Feb 15, 2017 at 2:39 PM Richard Braun <[hidden email]> wrote:
On Wed, Feb 15, 2017 at 10:00:25AM +0000, Manuel Klimek wrote:
> Both of these projects duplicate a lot of code from clang-tidy and the AST
> matcher infrastructure in clang.
>
> I'd be curious what the reasons are to not use those, so we can try to
> address them :)

I tried using matchers at some point, but couldn't understand the
processing flow and felt a lot more comfortable with the current result
despite writing more code.

Thanks for the insight. I hope that we have addressed some of the problems with clang-query in the meantime, but I take from this that we'll still need to improve the docs.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: The QtReslot Clang plugin

David Chisnall via cfe-dev
In reply to this post by David Chisnall via cfe-dev
On Tue, Feb 14, 2017 at 7:33 PM, Richard Braun via cfe-dev
<[hidden email]> wrote:
> Hello,
>
> During work on an internal Qt-based project at Novasys-Ingenierie,
> a Clang plugin was written to convert string-based signals and slots
> to the Qt5 syntax. The plugin has since been pushed to Github [1]
> in the hope it will be useful to others facing similar issues.

Hi Richard,

Beware that the new PMF syntax is not a drop in replacement for the old style.
Applying these fixits on a codebase might introduce bugs.

Here's some things to watch out for:

- You can't disconnect with new-syntax if the connect was made with
old-syntax (and vice-versa)
- Difference in behaviour when calling slots of partially destroyed
objects (explained in <https://codereview.qt-project.org/#/c/83800>,
not sure if it will be merged. <- Thiago ??)
- Old style is capable of making functions "virtual" without using the
virtual keyword.


Just for fun, can you run your plugin on clazy's unit-tests
(clazy/tests/old-style-connect/*.cpp) and compared them to
*fixed.cpp.expected, to see if it converted the connects correctly ?


Regards,
Sérgio Martins
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: The QtReslot Clang plugin

David Chisnall via cfe-dev
Hey all,

would it make sense to add these tools to the External Clang Examples page? Which is a file in the `docs` directory of the Clang repo.

Regards,
Laszlo

On Thu, Feb 16, 2017 at 4:55 AM, Sérgio Martins via cfe-dev <[hidden email]> wrote:
On Tue, Feb 14, 2017 at 7:33 PM, Richard Braun via cfe-dev
<[hidden email]> wrote:
> Hello,
>
> During work on an internal Qt-based project at Novasys-Ingenierie,
> a Clang plugin was written to convert string-based signals and slots
> to the Qt5 syntax. The plugin has since been pushed to Github [1]
> in the hope it will be useful to others facing similar issues.

Hi Richard,

Beware that the new PMF syntax is not a drop in replacement for the old style.
Applying these fixits on a codebase might introduce bugs.

Here's some things to watch out for:

- You can't disconnect with new-syntax if the connect was made with
old-syntax (and vice-versa)
- Difference in behaviour when calling slots of partially destroyed
objects (explained in <https://codereview.qt-project.org/#/c/83800>,
not sure if it will be merged. <- Thiago ??)
- Old style is capable of making functions "virtual" without using the
virtual keyword.


Just for fun, can you run your plugin on clazy's unit-tests
(clazy/tests/old-style-connect/*.cpp) and compared them to
*fixed.cpp.expected, to see if it converted the connects correctly ?


Regards,
Sérgio Martins
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: The QtReslot Clang plugin

David Chisnall via cfe-dev
In reply to this post by David Chisnall via cfe-dev
On Wed, Feb 15, 2017 at 05:55:13PM +0000, Sérgio Martins wrote:
> Beware that the new PMF syntax is not a drop in replacement for the old style.
> Applying these fixits on a codebase might introduce bugs.

Yes, the user is clearly made aware of this.

> Just for fun, can you run your plugin on clazy's unit-tests
> (clazy/tests/old-style-connect/*.cpp) and compared them to
> *fixed.cpp.expected, to see if it converted the connects correctly ?

Well clazy is a lot better it seems. I didn't take care of explicit
namespaces (although there is a TODO entry about that). QtReslot
also can't deal with wrong code, it merely converts, so things like
using SIGNAL() for a slot are caught, and a warning is emitted, but
nothing more.

On the other hand, clazy also seems a lot more intrusive, but I suppose
it's worth it considering the context. I wish I knew it existed and
could do such conversions before I started QtReslot, although I can't
give any advice on how to increase awareness about it.

--
Richard Braun
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: The QtReslot Clang plugin

David Chisnall via cfe-dev
On Thursday, 16 February 2017 11:24:17 CET Richard Braun via cfe-dev wrote:

> On Wed, Feb 15, 2017 at 05:55:13PM +0000, Sérgio Martins wrote:
> > Beware that the new PMF syntax is not a drop in replacement for the old
> > style. Applying these fixits on a codebase might introduce bugs.
>
> Yes, the user is clearly made aware of this.
>
> > Just for fun, can you run your plugin on clazy's unit-tests
> > (clazy/tests/old-style-connect/*.cpp) and compared them to
> > *fixed.cpp.expected, to see if it converted the connects correctly ?
>
> Well clazy is a lot better it seems. I didn't take care of explicit
> namespaces (although there is a TODO entry about that). QtReslot
> also can't deal with wrong code, it merely converts, so things like
> using SIGNAL() for a slot are caught, and a warning is emitted, but
> nothing more.
>
> On the other hand, clazy also seems a lot more intrusive, but I suppose
> it's worth it considering the context. I wish I knew it existed and
> could do such conversions before I started QtReslot, although I can't
> give any advice on how to increase awareness about it.
Regarding awareness, I really think we need to work on that a bit. Clazy is
indeed not discoverable via most of the terms I could come up with when
googling for a tool *like* Clazy.

E.g.:

- "qt automated refactoring" -> mostly hits about QtCreator
qt automated refactoring
- "qt automated porting" -> Qt4->Qt5 KDAB blog, etc.
- "qt static analysis" -> Qt Creator again
- "qt static analysis clang" -> even there, just hits about Clang SA & QtC

Tips for solutions:

1) Clazy's README [1] needs to be a bit more 'click-baity'. We need to have
'porting', 'refactoring', 'static', 'analysis', etc. in there. Note an
introduction is completely missing in that README
2) Scatter more keywords in our blogs about clazy
3) Even more blogging ;)

Cheers,
Kevin

[1] https://phabricator.kde.org/source/clazy/browse/master/README.md

--
Kevin Funk | [hidden email] | http://kfunk.org
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

signature.asc (169 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: The QtReslot Clang plugin

David Chisnall via cfe-dev
In reply to this post by David Chisnall via cfe-dev


On Wed, Feb 15, 2017 at 5:39 AM, Richard Braun via cfe-dev <[hidden email]> wrote:
On Wed, Feb 15, 2017 at 10:00:25AM +0000, Manuel Klimek wrote:
> Both of these projects duplicate a lot of code from clang-tidy and the AST
> matcher infrastructure in clang.
>
> I'd be curious what the reasons are to not use those, so we can try to
> address them :)

I tried using matchers at some point, but couldn't understand the
processing flow and felt a lot more comfortable with the current result
despite writing more code.


The thing that made it click for me is that the way it works is basically a classic "tree interpreter" type pattern. I wrote up some ideas for improving the documentation a long time ago when I was staring at it, though I don't think it ever made it to the mailing list (oops!).


I think this started out as a reply in an email and then I saved it into a document as it morphed into ideas for implementing the dynamic AST matches in a way that doesn't require stamping out huge amounts of text (as in machine code; essentially it instantiates each matcher separately resulting in huge .o files instead of using a "data-driven" approach based on the node type enums). Don't really clearly remember though.

In a more traditional OO implementation of the tree interpreter pattern it would look something like (pseudocode):


class MatcherBase {
  std::vector<std::unique_ptr<MatcherBase>> Children;

  virtual bool matches(ASTNode &Node) = 0;

...

}

class AllOfMatcher : public MatcherBase {
  bool matches(ASTNode &Node) override {
    for (auto &Matcher : Children)
      if (!Matcher->matches(Node))
        return false;
    return true;
  }

...

}

std::unique_ptr<MatcherBase> allOf(std::vector<std::unique_ptr<MatcherBase>> Children) {
  return make_unique<AllOfMatcher>(Children);
}

class CallExprMatcher : public AllOfMatcher {
  bool matches(ASTNode &Node) override {
    if (!AllOfMatcher::matches(Node))
      return false;
    return isa<CallExpr(Node);
  }

...

}


std::unique_ptr<MatcherBase> callExpr(std::vector<std::unique_ptr<MatcherBase>> Children) {
  return make_unique<CallExprMatcher>(Children);
}


The implementation actually looks essentially like this, except that it is static instead of dynamic and so the body of the `matches` virtual method essentially ends up inside templates and macros somewhere and isn't actually a virtual function)

For example, if you look at the implementation of the `callExpr` matcher, you see:

```
const internal::VariadicDynCastAllOfMatcher<Stmt, CallExpr> callExpr;
```

"dyn_cast AllOfMatcher" does pretty much what you expect based on looking at the toy OO pseudocode above. (the need to pass the Stmt template argument is not that relevant; it could probably be inferred by following the inheritance chain from CallExpr up to the top of the inheritance hierarchy)




(I've been totally out of the loop for quite a while for all the stuff that Manuel et al. have been doing with Clang tooling; so all of this is subject to being out of date or just remembered incorrectly)

-- Sean Silva
 

--
Richard Braun
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: The QtReslot Clang plugin

David Chisnall via cfe-dev
In reply to this post by David Chisnall via cfe-dev
On Thursday, 16 February 2017 19:26:48 CET Laszlo Nagy via cfe-dev wrote:
> Hey all,
>
> would it make sense to add these tools to the External Clang Examples
> <https://clang.llvm.org/docs/ExternalClangExamples.html> page? Which is a
> file in the `docs` directory of the Clang repo.

+1.

https://reviews.llvm.org/D30252

Cheers,
Kevin
 

> Regards,
> Laszlo
>
> On Thu, Feb 16, 2017 at 4:55 AM, Sérgio Martins via cfe-dev <
>
> [hidden email]> wrote:
> > On Tue, Feb 14, 2017 at 7:33 PM, Richard Braun via cfe-dev
> >
> > <[hidden email]> wrote:
> > > Hello,
> > >
> > > During work on an internal Qt-based project at Novasys-Ingenierie,
> > > a Clang plugin was written to convert string-based signals and slots
> > > to the Qt5 syntax. The plugin has since been pushed to Github [1]
> > > in the hope it will be useful to others facing similar issues.
> >
> > Hi Richard,
> >
> > Beware that the new PMF syntax is not a drop in replacement for the old
> > style.
> > Applying these fixits on a codebase might introduce bugs.
> >
> > Here's some things to watch out for:
> >
> > - You can't disconnect with new-syntax if the connect was made with
> > old-syntax (and vice-versa)
> > - Difference in behaviour when calling slots of partially destroyed
> > objects (explained in <https://codereview.qt-project.org/#/c/83800>,
> > not sure if it will be merged. <- Thiago ??)
> > - Old style is capable of making functions "virtual" without using the
> > virtual keyword.
> >
> >
> > Just for fun, can you run your plugin on clazy's unit-tests
> > (clazy/tests/old-style-connect/*.cpp) and compared them to
> > *fixed.cpp.expected, to see if it converted the connects correctly ?
> >
> >
> > Regards,
> > Sérgio Martins
> > _______________________________________________
> > cfe-dev mailing list
> > [hidden email]
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

--
Kevin Funk | [hidden email] | http://kfunk.org
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

signature.asc (169 bytes) Download Attachment