Where to put upcoming refcatoring tools

Hi guys,

I'm remembering discussions about which set of tools we want to be in
the clang mainline, and for which we might want to create a new
project.
Since the tooling base has landed now, I'd like to get some idea of
how we're planning to structure the clang based tools in the code
base, so we have clear guidelines for the future.

It seems like on the one hand there's clang-check, which is so closely
coupled to clang itself that my gut feeling is it fits perfectly into
the clang mainline.
On the other side we have things like 'clang-extract-method', which
we'll probably not want in clang mainline.
In between there are things like clang-format, which basically all
tools that do code changes will depend upon, thus it's still pretty
close to clang, so we might want to put that into mainline, but I'm
not sure.

My proposal would be to create a new top-level llvm project for the
refactoring tools with a clear guideline on what would go in there.

Thoughts?

Thanks,
/Manuel

Manuel Klimek wrote:

Hi guys,

I'm remembering discussions about which set of tools we want to be in
the clang mainline, and for which we might want to create a new
project.
Since the tooling base has landed now, I'd like to get some idea of
how we're planning to structure the clang based tools in the code
base, so we have clear guidelines for the future.

It seems like on the one hand there's clang-check, which is so closely
coupled to clang itself that my gut feeling is it fits perfectly into
the clang mainline.
On the other side we have things like 'clang-extract-method', which
we'll probably not want in clang mainline.
In between there are things like clang-format, which basically all
tools that do code changes will depend upon, thus it's still pretty
close to clang, so we might want to put that into mainline, but I'm
not sure.

My proposal would be to create a new top-level llvm project for the
refactoring tools with a clear guideline on what would go in there.

Thoughts?

Thanks,
/Manuel

Did anything come of this? I'm trying to find out how to write a clang C++
rewriting tool.

Not yet. Currently the majority of the refactoring infrastructure
(besides the basic tooling support) is in a branch in
^cfe/branches/tooling. There are ways to write rewrite tools based on
what's in mainline, it just requires a little more busy work on your
side.

Out of curiosity: what kind of rewriting tool are you planning to implement?

Cheers,
/Manuel

Manuel Klimek wrote:

Manuel Klimek wrote:

Hi guys,

I'm remembering discussions about which set of tools we want to be in
the clang mainline, and for which we might want to create a new
project.
Since the tooling base has landed now, I'd like to get some idea of
how we're planning to structure the clang based tools in the code
base, so we have clear guidelines for the future.

It seems like on the one hand there's clang-check, which is so closely
coupled to clang itself that my gut feeling is it fits perfectly into
the clang mainline.
On the other side we have things like 'clang-extract-method', which
we'll probably not want in clang mainline.
In between there are things like clang-format, which basically all
tools that do code changes will depend upon, thus it's still pretty
close to clang, so we might want to put that into mainline, but I'm
not sure.

My proposal would be to create a new top-level llvm project for the
refactoring tools with a clear guideline on what would go in there.

Thoughts?

Thanks,
/Manuel

Did anything come of this? I'm trying to find out how to write a clang
C++ rewriting tool.

Not yet. Currently the majority of the refactoring infrastructure
(besides the basic tooling support) is in a branch in
^cfe/branches/tooling. There are ways to write rewrite tools based on
what's in mainline, it just requires a little more busy work on your
side.

That doesn't seem to be in the git repo as far as I can tell. However, I
checked out an old revision which has the ReplaceCStrCalls tool, and I read
through the MatchFinder API yesterday. I haven't tried it out yet though. I
presume that's what's in the branch?

Out of curiosity: what kind of rewriting tool are you planning to
implement?

I'm interested in automated semantic porting from Qt 4 to Qt 5 (I'm a KDE
contributor and Qt consultant with KDAB).

To try out the tool, my first goal is to be able to port

namespace Qt {
QString escape(const QString &input);
}

which is deprecated in Qt 5 and should be replaced with a use of

class QString {
  ...
  QString toHtmlEscaped() const;
};

It gets interesting because there are several implicit constructors for
QString.

Qt::escape("foo");

needs to be changed to

QString::fromLatin1("foo").toHtmlEscaped();

or one of several other constructs for creating QStrings.

Then there are things like

Qt::escape(someAPIReturningQString());

which should be

someAPIReturningQString().toHtmlEscaped();

and

Qt::escape(someAPIReturningQByteArray());

(or APIs returning a const char * for example)

which would need to be ported to:

QString::fromLatin1(someAPIReturningQByteArray()).toHtmlEscaped();

(the implicit conversion from QByteArray to QString uses a latin1 codec)

That is, the porting tool which finds a use of Qt::escape needs to know if
the QString argument is the result of an implicit cast or not. There are
many other porting cases in Qt 4 to 5 where implicit casts are involved, or
which would benefit from semantic porting rather than python or sed scripts.

There are also similar cases to deal with in porting KDE applications.

Let me know if you have any tips or guidance on how to get started. I'm
hoping the example here
http://thread.gmane.org/gmane.comp.compilers.llvm.bugs/17991 which looks
compilable will lead me to figure out how to add in-place rewriting to the
tool.

Thanks,

Steve.

Manuel Klimek wrote:

Manuel Klimek wrote:

Hi guys,

I'm remembering discussions about which set of tools we want to be in
the clang mainline, and for which we might want to create a new
project.
Since the tooling base has landed now, I'd like to get some idea of
how we're planning to structure the clang based tools in the code
base, so we have clear guidelines for the future.

It seems like on the one hand there's clang-check, which is so closely
coupled to clang itself that my gut feeling is it fits perfectly into
the clang mainline.
On the other side we have things like 'clang-extract-method', which
we'll probably not want in clang mainline.
In between there are things like clang-format, which basically all
tools that do code changes will depend upon, thus it's still pretty
close to clang, so we might want to put that into mainline, but I'm
not sure.

My proposal would be to create a new top-level llvm project for the
refactoring tools with a clear guideline on what would go in there.

Thoughts?

Thanks,
/Manuel

Did anything come of this? I'm trying to find out how to write a clang
C++ rewriting tool.

Not yet. Currently the majority of the refactoring infrastructure
(besides the basic tooling support) is in a branch in
^cfe/branches/tooling. There are ways to write rewrite tools based on
what's in mainline, it just requires a little more busy work on your
side.

That doesn't seem to be in the git repo as far as I can tell. However, I
checked out an old revision which has the ReplaceCStrCalls tool, and I read
through the MatchFinder API yesterday. I haven't tried it out yet though. I
presume that's what's in the branch?

The MatchFinder API and the RefactoringTool API
(include/clang/Tooling/Refactoring.h).

The MatchFinder API would be useful in your case to find the cases you
want to rewrite. The RefactoringTool API makes it possible to run over
many TUs in one process and reply all the edits after deduplicating
them, leaving your repository in a compilable state.

The ReplaceCStrCalls example is definitely the best one we currently
have for refactorings.

Out of curiosity: what kind of rewriting tool are you planning to
implement?

I'm interested in automated semantic porting from Qt 4 to Qt 5 (I'm a KDE
contributor and Qt consultant with KDAB).

To try out the tool, my first goal is to be able to port

namespace Qt {
QString escape(const QString &input);
}

which is deprecated in Qt 5 and should be replaced with a use of

class QString {
...
QString toHtmlEscaped() const;
};

It gets interesting because there are several implicit constructors for
QString.

Qt::escape("foo");

needs to be changed to

QString::fromLatin1("foo").toHtmlEscaped();

or one of several other constructs for creating QStrings.

Then there are things like

Qt::escape(someAPIReturningQString());

which should be

someAPIReturningQString().toHtmlEscaped();

and

Qt::escape(someAPIReturningQByteArray());

(or APIs returning a const char * for example)

which would need to be ported to:

QString::fromLatin1(someAPIReturningQByteArray()).toHtmlEscaped();

(the implicit conversion from QByteArray to QString uses a latin1 codec)

That is, the porting tool which finds a use of Qt::escape needs to know if
the QString argument is the result of an implicit cast or not. There are
many other porting cases in Qt 4 to 5 where implicit casts are involved, or
which would benefit from semantic porting rather than python or sed scripts.

There are also similar cases to deal with in porting KDE applications.

Let me know if you have any tips or guidance on how to get started. I'm
hoping the example here
http://thread.gmane.org/gmane.comp.compilers.llvm.bugs/17991 which looks
compilable will lead me to figure out how to add in-place rewriting to the
tool.

This is exactly the example our tooling / ASTMatcher infrastructure is
written for (and we have similar examples we do internally for our
APIs). If you want to try to put up a tool based on the tooling
branch, feel free to contact me when you need help or have feedback on
the APIs.

Cheers,
/Manuel

Manuel Klimek wrote:

Did anything come of this? I'm trying to find out how to write a clang
C++ rewriting tool.

Not yet. Currently the majority of the refactoring infrastructure
(besides the basic tooling support) is in a branch in
^cfe/branches/tooling. There are ways to write rewrite tools based on
what's in mainline, it just requires a little more busy work on your
side.

That doesn't seem to be in the git repo as far as I can tell. However, I
checked out an old revision which has the ReplaceCStrCalls tool, and I
read through the MatchFinder API yesterday. I haven't tried it out yet
though. I presume that's what's in the branch?

The MatchFinder API and the RefactoringTool API
(include/clang/Tooling/Refactoring.h).

The MatchFinder API would be useful in your case to find the cases you
want to rewrite. The RefactoringTool API makes it possible to run over
many TUs in one process and reply all the edits after deduplicating
them, leaving your repository in a compilable state.

The ReplaceCStrCalls example is definitely the best one we currently
have for refactorings.

I tried building this branch with llvm trunk and got this error:

Linking CXX static library
../../../../../lib/libclangStaticAnalyzerFrontend.a
[ 89%] Built target clangStaticAnalyzerFrontend
[ 89%] Building CXX object
tools/clang/lib/CodeGen/CMakeFiles/clangCodeGen.dir/CGDebugInfo.cpp.o
/home/stephen/dev/src/llvm/tools/clang/lib/CodeGen/CGDebugInfo.cpp: In
member function 'llvm::DIType clang::CodeGen::CGDebugInfo::CreateType(const
clang::BuiltinType*)':
/home/stephen/dev/src/llvm/tools/clang/lib/CodeGen/CGDebugInfo.cpp:354:40:
error: no matching function for call to
'llvm::DIBuilder::createForwardDecl(llvm::dwarf::dwarf_constants, const char
[11], llvm::DIFile, int)'
/home/stephen/dev/src/llvm/tools/clang/lib/CodeGen/CGDebugInfo.cpp:354:40:
note: candidate is:
/home/stephen/dev/src/llvm/include/llvm/Analysis/DIBuilder.h:351:12: note:
llvm::DIType llvm::DIBuilder::createForwardDecl(unsigned int,
llvm::StringRef, llvm::DIDescriptor, llvm::DIFile, unsigned int, unsigned
int)
/home/stephen/dev/src/llvm/include/llvm/Analysis/DIBuilder.h:351:12: note:
candidate expects 6 arguments, 4 provided
/home/stephen/dev/src/llvm/tools/clang/lib/CodeGen/CGDebugInfo.cpp:365:35:
error: no matching function for call to
'llvm::DIBuilder::createForwardDecl(llvm::dwarf::dwarf_constants, const char
[11], llvm::DIFile, int)'
/home/stephen/dev/src/llvm/tools/clang/lib/CodeGen/CGDebugInfo.cpp:365:35:
note: candidate is:
/home/stephen/dev/src/llvm/include/llvm/Analysis/DIBuilder.h:351:12: note:
llvm::DIType llvm::DIBuilder::createForwardDecl(unsigned int,
llvm::StringRef, llvm::DIDescriptor, llvm::DIFile, unsigned int, unsigned
int)
/home/stephen/dev/src/llvm/include/llvm/Analysis/DIBuilder.h:351:12: note:
candidate expects 6 arguments, 4 provided
/home/stephen/dev/src/llvm/tools/clang/lib/CodeGen/CGDebugInfo.cpp:386:35:
error: no matching function for call to
'llvm::DIBuilder::createForwardDecl(llvm::dwarf::dwarf_constants, const char
[14], llvm::DIFile, int)'
/home/stephen/dev/src/llvm/tools/clang/lib/CodeGen/CGDebugInfo.cpp:386:35:
note: candidate is:
/home/stephen/dev/src/llvm/include/llvm/Analysis/DIBuilder.h:351:12: note:
llvm::DIType llvm::DIBuilder::createForwardDecl(unsigned int,
llvm::StringRef, llvm::DIDescriptor, llvm::DIFile, unsigned int, unsigned
int)
/home/stephen/dev/src/llvm/include/llvm/Analysis/DIBuilder.h:351:12: note:
candidate expects 6 arguments, 4 provided
/home/stephen/dev/src/llvm/tools/clang/lib/CodeGen/CGDebugInfo.cpp: In
member function 'llvm::DIType
clang::CodeGen::CGDebugInfo::createRecordFwdDecl(const clang::RecordDecl*,
llvm::DIDescriptor)':
/home/stephen/dev/src/llvm/tools/clang/lib/CodeGen/CGDebugInfo.cpp:517:63:
error: no matching function for call to
'llvm::DIBuilder::createForwardDecl(unsigned int&, llvm::StringRef&,
llvm::DIFile&, unsigned int&)'
/home/stephen/dev/src/llvm/tools/clang/lib/CodeGen/CGDebugInfo.cpp:517:63:
note: candidate is:
/home/stephen/dev/src/llvm/include/llvm/Analysis/DIBuilder.h:351:12: note:
llvm::DIType llvm::DIBuilder::createForwardDecl(unsigned int,
llvm::StringRef, llvm::DIDescriptor, llvm::DIFile, unsigned int, unsigned
int)
/home/stephen/dev/src/llvm/include/llvm/Analysis/DIBuilder.h:351:12: note:
candidate expects 6 arguments, 4 provided
/home/stephen/dev/src/llvm/tools/clang/lib/CodeGen/CGDebugInfo.cpp: In
member function 'llvm::DIType clang::CodeGen::CGDebugInfo::CreateType(const
clang::ObjCInterfaceType*, llvm::DIFile)':
/home/stephen/dev/src/llvm/tools/clang/lib/CodeGen/CGDebugInfo.cpp:1291:17:
error: no matching function for call to
'llvm::DIBuilder::createForwardDecl(llvm::dwarf::dwarf_constants,
llvm::StringRef, llvm::DIFile&, unsigned int&, unsigned int&)'
/home/stephen/dev/src/llvm/tools/clang/lib/CodeGen/CGDebugInfo.cpp:1291:17:
note: candidate is:
/home/stephen/dev/src/llvm/include/llvm/Analysis/DIBuilder.h:351:12: note:
llvm::DIType llvm::DIBuilder::createForwardDecl(unsigned int,
llvm::StringRef, llvm::DIDescriptor, llvm::DIFile, unsigned int, unsigned
int)
/home/stephen/dev/src/llvm/include/llvm/Analysis/DIBuilder.h:351:12: note:
no known conversion for argument 4 from 'unsigned int' to 'llvm::DIFile'
/home/stephen/dev/src/llvm/tools/clang/lib/CodeGen/CGDebugInfo.cpp: In
member function 'llvm::DIType
clang::CodeGen::CGDebugInfo::createRecordFwdDecl(const clang::RecordDecl*,
llvm::DIDescriptor)':
/home/stephen/dev/src/llvm/tools/clang/lib/CodeGen/CGDebugInfo.cpp:518:1:
warning: control reaches end of non-void function [-Wreturn-type]
make[2]: ***
[tools/clang/lib/CodeGen/CMakeFiles/clangCodeGen.dir/CGDebugInfo.cpp.o]
Error 1
make[1]: *** [tools/clang/lib/CodeGen/CMakeFiles/clangCodeGen.dir/all] Error
2
make: *** [all] Error 2

Could you give me a llvm commit I'd be able to build against? Or a patch to
make it build again (I guess the extra needed arguments have to come from
somewhere)?

Thanks,

Steve.

Stephen Kelly wrote:

Could you give me a llvm commit I'd be able to build against? Or a patch
to make it build again (I guess the extra needed arguments have to come
from somewhere)?

I commented out the conflicting code, and now I get this when trying to run
the example:

stephen@hal:~/dev/src/llvm{master}$ ~/dev/build/qt/llvm/bin/remove-cstr-
calls ~/dev/build/qt/llvm/ ./unittests/VMCore/DominatorTreeTest.cpp
Processing:
/home/stephen/dev/src/llvm/unittests/VMCore/DominatorTreeTest.cpp.
In file included from
/home/stephen/dev/src/llvm/unittests/VMCore/DominatorTreeTest.cpp:1:
In file included from
/home/stephen/dev/src/llvm/include/llvm/Instructions.h:19:
In file included from
/home/stephen/dev/src/llvm/include/llvm/InstrTypes.h:19:
In file included from
/home/stephen/dev/src/llvm/include/llvm/Instruction.h:18:
In file included from /home/stephen/dev/src/llvm/include/llvm/User.h:22:
In file included from
/home/stephen/dev/src/llvm/include/llvm/Support/ErrorHandling.h:19:
In file included from
/home/stephen/dev/src/llvm/include/llvm/ADT/StringRef.h:13:
In file included from
/home/stephen/dev/src/llvm/include/llvm/Support/type_traits.h:20:
In file included from
/home/stephen/dev/build/qt/llvm/include/llvm/Support/DataTypes.h:58:
/usr/include/x86_64-linux-gnu/sys/types.h:147:10: fatal error: 'stddef.h'
file not found
#include <stddef.h>
         ^
1 error generated.
Error while processing
/home/stephen/dev/src/llvm/unittests/VMCore/DominatorTreeTest.cpp.

Is there more set-up I need to do?

Yes, but there shouldn't, so you just pointed me at a bug in the
tooling which I didn't notice because it's easily hidden behind a
clang system install :smiley:

You can work around this for now by making sure that the compiler that
you configured as the compiler in the project where you created the
compile_commands.json (more specifically, the argv[0] in the "command"
entries in that file) is a clang installation where
basepath(argv[0])/../lib/clang/3.2/include (or 3.1 depending on
whether you have a clang version pre-cut) exists.

The bug is that the tooling hands the full compile command line from
the compile_commands.json to the driver, which uses the argv[0] of
that to find its builtin includes. The fix is to replace this with the
current tool's argv[0] for the calls.

Cheers,
/Manuel

Manuel Klimek wrote:

You can work around this for now by making sure that the compiler that
you configured as the compiler in the project where you created the
compile_commands.json (more specifically, the argv[0] in the "command"
entries in that file) is a clang installation where
basepath(argv[0])/../lib/clang/3.2/include (or 3.1 depending on
whether you have a clang version pre-cut) exists.

The bug is that the tooling hands the full compile command line from
the compile_commands.json to the driver, which uses the argv[0] of
that to find its builtin includes. The fix is to replace this with the
current tool's argv[0] for the calls.

Great, thanks.

I got the example running and working, so now I will see what more I can
learn about the tooling APIs. I've had a read through ASTMatchers.h, so that
looks like a good start.

Thanks,

Steve.

Manuel Klimek wrote:

You can work around this for now by making sure that the compiler that
you configured as the compiler in the project where you created the
compile_commands.json (more specifically, the argv[0] in the "command"
entries in that file) is a clang installation where
basepath(argv[0])/../lib/clang/3.2/include (or 3.1 depending on
whether you have a clang version pre-cut) exists.

The bug is that the tooling hands the full compile command line from
the compile_commands.json to the driver, which uses the argv[0] of
that to find its builtin includes. The fix is to replace this with the
current tool's argv[0] for the calls.

Great, thanks.

I got the example running and working, so now I will see what more I can
learn about the tooling APIs. I've had a read through ASTMatchers.h, so that
looks like a good start.

Fixed in mainline and merged the tooling branch. Should all work like
expected now.

Cheers,
/Manuel

Manuel Klimek wrote:

I got the example running and working, so now I will see what more I can
learn about the tooling APIs. I've had a read through ASTMatchers.h, so
that looks like a good start.

Fixed in mainline and merged the tooling branch. Should all work like
expected now.

Great, thanks.

I haven't tried it yet, but will do so soon.

Meanwhile, I have hit a few problems trying to modify remove-cstr-calls to
port my Qt API calls.

Firstly, I'm using some printf debugging and trial and error to try to get
the code working. Is there a better way?

My testcase is this:

  Qt::escape("Foo"); // Use implicit QString(const char *)
  QString foo("bar");
  Qt::escape(foo); // Use QString copy ctor.
  Qt::escape(qApp->objectName()); // Use QString copy ctor.
  // Use implicit QString(QByteArray) :
  Qt::escape(qApp->objectName().toLatin1());

My first attempt at a matcher was:

  Finder.addMatcher(
      Id("call",
        Call(
          Callee(Function(HasName("escape")))
          , HasArgument(
              0, Id("arg", Expression())
          )
        )
      ),
      &Callback);

However, I need to handle the calls differently based on whether or what
kind of implicit cast is used to call Qt::escape(). I tried to refine my
matcher to match only the calls where the expression is already of type
QString (ie, needs no implicit cast):

              0, Id("arg", Expression(HasType(Class(HasName("QString")))))

This refinement didn't work, giving the same matches as before. Is this the
wrong approach entirely? Is it not possible to match like that? Should I
just let it match everything and conditionally change what I replace the
calls with inside the MatchCallback::run implementation instead?

Additionally, the third and fourth uses of escape are excluded by the
getText function because it determines that it starts and ends in different
files. The reason for that seems to be that qApp is actually a macro, which
can expand to QCoreApplication::instance() or QApplication::instance();

Using this matches as expected:

  QObject o;
  Qt::escape(o.objectName()); // Use QString copy ctor.
  // Use implicit QString(QByteArray) :
  Qt::escape(o.objectName().toLatin1());

So, really the only thing I need to find out is how to determine whether an
implicit cast is being used in the Call or not.

Thanks,

Steve.

Manuel Klimek wrote:

I got the example running and working, so now I will see what more I can
learn about the tooling APIs. I've had a read through ASTMatchers.h, so
that looks like a good start.

Fixed in mainline and merged the tooling branch. Should all work like
expected now.

Great, thanks.

I haven't tried it yet, but will do so soon.

Meanwhile, I have hit a few problems trying to modify remove-cstr-calls to
port my Qt API calls.

Firstly, I'm using some printf debugging and trial and error to try to get
the code working. Is there a better way?

Unfortunately not - I'm open to ideas :smiley:
One thing I'm usually doing is to create small unit tests that execute
particular cases and bootstrap from "obviously working" towards a more
and more restrictive filter.

My testcase is this:

Qt::escape("Foo"); // Use implicit QString(const char *)
QString foo("bar");
Qt::escape(foo); // Use QString copy ctor.
Qt::escape(qApp->objectName()); // Use QString copy ctor.
// Use implicit QString(QByteArray) :
Qt::escape(qApp->objectName().toLatin1());

My first attempt at a matcher was:

Finder.addMatcher(
Id("call",
Call(
Callee(Function(HasName("escape")))
, HasArgument(
0, Id("arg", Expression())
)
)
),
&Callback);

However, I need to handle the calls differently based on whether or what
kind of implicit cast is used to call Qt::escape(). I tried to refine my
matcher to match only the calls where the expression is already of type
QString (ie, needs no implicit cast):

         0, Id\(&quot;arg&quot;, Expression\(HasType\(Class\(HasName\(&quot;QString&quot;\)\)\)\)\)

This refinement didn't work, giving the same matches as before. Is this the
wrong approach entirely? Is it not possible to match like that? Should I
just let it match everything and conditionally change what I replace the
calls with inside the MatchCallback::run implementation instead?

Does
Expresssion(AllOf(
  HasType(...),
  Not(ImplicitCast())
work?

Additionally, the third and fourth uses of escape are excluded by the
getText function because it determines that it starts and ends in different
files. The reason for that seems to be that qApp is actually a macro, which
can expand to QCoreApplication::instance() or QApplication::instance();

Yea, the getText function needs some love - the problem is that you
need either the spelling or the expansion location based on what you
want to look at - in this particular instance you'll want the
expansion location.

Let me know if you have more questions!
Cheers,
/Manuel

Manuel Klimek wrote:

Does
Expresssion(AllOf(
  HasType(...),
  Not(ImplicitCast())
work?

Unfortunately not. The Not(ImplicitCast()) also makes no difference.

I tried adding a ConstructorCall() in there too, guessing that would match
the implicit ctor calls, but that just excluded all matches from the result.

I put a call to Arg->dump() in my MatchCallback implementation. It doesn't
help me, but maybe you can see how I need to define my match with that? My
input is this now:

    Qt::escape("foo");
    QString foo("bar");
    Qt::escape(foo);
    QObject o;
    Qt::escape(o.objectName());
    Qt::escape(o.property("foo").toByteArray());

Thanks,

Steve.

output (2.25 KB)

Stephen Kelly wrote:

Manuel Klimek wrote:

Does
Expresssion(AllOf(
  HasType(...),
  Not(ImplicitCast())
work?

Unfortunately not. The Not(ImplicitCast()) also makes no difference.

I tried adding a ConstructorCall() in there too, guessing that would match
the implicit ctor calls, but that just excluded all matches from the
result.

I put a call to Arg->dump() in my MatchCallback implementation. It doesn't
help me, but maybe you can see how I need to define my match with that? My
input is this now:

    Qt::escape("foo");
    QString foo("bar");
    Qt::escape(foo);
    QObject o;
    Qt::escape(o.objectName());
    Qt::escape(o.property("foo").toByteArray());

I found that I could use this to not match the Qt::escape(foo); case.

  Finder.addMatcher(
      Id("call",
        Call(
          Callee(Function(HasName("::Qt::escape"))),
          HasArgument(
            0,
            Id("arg", BindTemporaryExpression())
          )
        )
      ),
      &Callback);
  
I eventually figured out how to limit it to only the case without
temporaries:

  Finder.addMatcher(
      Id("call",
        Call(
          Callee(Function(HasName("::Qt::escape"))),
          Not(HasArgument(
            0,
            Id("arg", BindTemporaryExpression())
          )),
          HasArgument(
            0,
            Id("arg", Expression())
          )
        )
      ),
      &Callback);

I tried many other variations using Not, which I can try to list if you're
interested in considering the self-documenting nature of the API.

To match the string literal, I used:

  Finder.addMatcher(
      Id("call",
        Call(
          Callee(Function(HasName("::Qt::escape"))),
          HasArgument(
            0,
            Id("arg", BindTemporaryExpression())
          ),
          Not(hasDescendant(MemberExpression()))
        )
      ),
      &Callback);

I could not use this as it gives no results:

  Finder.addMatcher(
      Id("call",
        Call(
          Callee(Function(HasName("::Qt::escape"))),
          HasArgument(
            0,
            Id("arg", BindTemporaryExpression())
          ),
          ast_matchers::StringLiteral()
        )
      ),
      &Callback);

And this one does not build:

  Finder.addMatcher(
      Id("call",
        Call(
          Callee(Function(HasName("::Qt::escape"))),
          HasArgument(
            0,
            Id("arg", BindTemporaryExpression())
          ),
          Not(hasDescendant(ast_matchers::StringLiteral()))
        )
      ),
      &Callback);

Finally this matches the member calls:

  Finder.addMatcher(
      Id("call",
        Call(
          Callee(Function(HasName("::Qt::escape"))),
          HasArgument(
            0,
            BindTemporaryExpression()
          ),
          hasDescendant(Id("arg", MemberExpression()))
        )
      ),
      &Callback);

But I do not get the parentheses or the arguments inside them in the result
(if any). I aslo presume this would not match a call of a free function, so
I would need more matchers for those cases? It's already getting quite
exhaustive and less scalable.

It seems either I'm missing something or the tooling is not ready to handle
the cases I'm trying to achieve?

Thanks,

Steve.

Stephen Kelly wrote:

Manuel Klimek wrote:

Does
Expresssion(AllOf(
HasType(...),
Not(ImplicitCast())
work?

Unfortunately not. The Not(ImplicitCast()) also makes no difference.

I tried adding a ConstructorCall() in there too, guessing that would match
the implicit ctor calls, but that just excluded all matches from the
result.

I put a call to Arg->dump() in my MatchCallback implementation. It doesn't
help me, but maybe you can see how I need to define my match with that? My
input is this now:

Qt::escape\(&quot;foo&quot;\);
QString foo\(&quot;bar&quot;\);
Qt::escape\(foo\);
QObject o;
Qt::escape\(o\.objectName\(\)\);
Qt::escape\(o\.property\(&quot;foo&quot;\)\.toByteArray\(\)\);

I found that I could use this to not match the Qt::escape(foo); case.

Finder.addMatcher(
Id("call",
Call(
Callee(Function(HasName("::Qt::escape"))),
HasArgument(
0,
Id("arg", BindTemporaryExpression())
)
)
),
&Callback);

I eventually figured out how to limit it to only the case without
temporaries:

Finder.addMatcher(
Id("call",
Call(
Callee(Function(HasName("::Qt::escape"))),
Not(HasArgument(
0,
Id("arg", BindTemporaryExpression())
)),
HasArgument(
0,
Id("arg", Expression())
)
)
),
&Callback);

I tried many other variations using Not, which I can try to list if you're
interested in considering the self-documenting nature of the API.

To match the string literal, I used:

Finder.addMatcher(
Id("call",
Call(
Callee(Function(HasName("::Qt::escape"))),
HasArgument(
0,
Id("arg", BindTemporaryExpression())
),
Not(hasDescendant(MemberExpression()))
)
),
&Callback);

I could not use this as it gives no results:

Finder.addMatcher(
Id("call",
Call(
Callee(Function(HasName("::Qt::escape"))),
HasArgument(
0,
Id("arg", BindTemporaryExpression())
),
ast_matchers::StringLiteral()
)
),
&Callback);

And this one does not build:

Finder.addMatcher(
Id("call",
Call(
Callee(Function(HasName("::Qt::escape"))),
HasArgument(
0,
Id("arg", BindTemporaryExpression())
),
Not(hasDescendant(ast_matchers::StringLiteral()))
)
),
&Callback);

Finally this matches the member calls:

Finder.addMatcher(
Id("call",
Call(
Callee(Function(HasName("::Qt::escape"))),
HasArgument(
0,
BindTemporaryExpression()
),
hasDescendant(Id("arg", MemberExpression()))
)
),
&Callback);

But I do not get the parentheses or the arguments inside them in the result
(if any). I aslo presume this would not match a call of a free function, so
I would need more matchers for those cases? It's already getting quite
exhaustive and less scalable.

It seems either I'm missing something or the tooling is not ready to handle
the cases I'm trying to achieve?

Ok, I'm confused by your last example. The way I usually approach this
is that I write a small cpp file with the stuff I want to match and
then run clang -ast-dump-xml over it, and look at the resulting AST.
Then you basically take the matchers that drill through the
corresponding AST nodes and put them together. I can take a look at
your specific example tomorrow (I just reread your first explanation
of what you're trying to do) and come up with an example how I would
approach this. That might also make up some good documentation :slight_smile:

Cheers,
/Manuel

Here's what I would do:
    Id("call",
      Call(Callee(Function(HasName("::Qt::escape"))),·
           HasArgument(0, AnyOf(Id("constructor", ConstructorCall()),
Id("expression", Expression())))))));

Now you get "constructor" bound when there's a conversion going on,
and "expression" bound otherwise.
If "constructor" is bound, replace "call" with
QString::fromUtf8(<constructor-range>), otherwise replace "call" with
(<expression>).toHtmlEscaped();

This works because HasArgument by default looks through parentheses
and implicit casts, and doing what looks like a manual constructor
call there (QString("")) is actually a functional cast.

Tweaking will be necessary if you want to prevent extra parens around
expression.
If the spelling location of <expression> or <constructor> still
doesn't work when you have a macro in there, you'll need to pimp the
getText function to also allow giving you the expansion location.

Cheers,
/Manuel