The QtReslot Clang plugin

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.

Hi!

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

Nikolai

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 :slight_smile:

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

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

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.

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&gt;,
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

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

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 :wink:

Cheers,
Kevin

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

> 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 :slight_smile:

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!).

https://docs.google.com/document/d/1foYulTnUXumiuGzzgoAbGQLFJerk4
X1WjR2cKBUUMJI/edit?usp=sharing

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

Hey all,

would it make sense to add these tools to the External Clang Examples
<https://clang.llvm.org/docs/ExternalClangExamples.html&gt; page? Which is a
file in the `docs` directory of the Clang repo.

+1.

https://reviews.llvm.org/D30252

Cheers,
Kevin