Clang plugin that rewrites PostTask(_, NewRunnableMethod(...)) to PostTask(_, base::Bind(...));

Clang plugin that rewrites PostTask(_, NewRunnableMethod(...)) to PostTask(_, base::Bind(...));

BUG=none
TEST=none

[awong, 2011-09-15 02:22:44]

And here we start with the scary migration code...

Nico, this is my first draft hackup of trying to convert NRM -> Bind.  This does
not yet correctly handle includes, or even write the output anywhere.  But I was
wondering if you could look at the AST walk and let me know if it even looked
sane.

Next up with this is to beef it up similar to ARC so that it can understand file
remapping, and then write the output to the actual source file.

-Albert

[awong, 2011-09-15 02:28:02]

Oh, and if you have any ideas about how to correctly re-indent the resulting
code, that'd be awesome.  At this point, I'm tempted to do it as a
post-processing step after migration.

On Wed, Sep 14, 2011 at 7:22 PM, <ajwong@chromium.org> wrote:

> Reviewers: Nico,
>
> Message:
> And here we start with the scary migration code...
>
> Nico, this is my first draft hackup of trying to convert NRM -> Bind.  This
> does
> not yet correctly handle includes, or even write the output anywhere.  But
> I was
> wondering if you could look at the AST walk and let me know if it even
> looked
> sane.
>
> Next up with this is to beef it up similar to ARC so that it can understand
> file
> remapping, and then write the output to the actual source file.
>
> -Albert
>
> Description:
> Clang plugin that rewrites PostTask(_, NewRunnableMethod(...)) to
> PostTask(_,
> base::Bind(...));
>
> BUG=none
> TEST=none
>
>
> Please review this at
http://codereview.chromium.**org/7886056/<http://codereview.chromium.org/7886...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  A tools/clang/TaskToBind/**CMakeLists.txt
>  A tools/clang/TaskToBind/**Makefile
>  A tools/clang/TaskToBind/README.**txt
>  A tools/clang/TaskToBind/**TaskToBind.cpp
>  A tools/clang/TaskToBind/**TaskToBind.exports
>
>
>

[Nico, 2011-09-15 02:50:02]

This looks like a great start.

For the relayout: Since all you do is replace text with shorter text and swap
elements, I think if you are careful with your replacements the layout should be
mostly conserved…can you give me an example where there are problems? But in
general, reindentation is a problem.

http://codereview.chromium.org/7886056/diff/1/tools/clang/TaskToBind/README.txt
File tools/clang/TaskToBind/README.txt (right):

http://codereview.chromium.org/7886056/diff/1/tools/clang/TaskToBind/README.t...
tools/clang/TaskToBind/README.txt:9: $ clang -cc1 -load
../../Debug+Asserts/lib/libPrintFunctionNames.so -plugin print-fns
some-input-file.c
print-fns?

http://codereview.chromium.org/7886056/diff/1/tools/clang/TaskToBind/README.t...
tools/clang/TaskToBind/README.txt:12: $ clang -cc1 -load
../../Debug+Asserts/lib/libPrintFunctionNames.dylib -plugin print-fns
some-input-file.c
Maybe mention


  GYP_DEFINES='clang=1 clang_load=/abs/path/to/libTaskToBind.dylib
clang_add_plugin=convert-postask' gclient runhooks

…then doing a clobber build will run your plugin on _all_ files.

http://codereview.chromium.org/7886056/diff/1/tools/clang/TaskToBind/TaskToBi...
File tools/clang/TaskToBind/TaskToBind.cpp (right):

http://codereview.chromium.org/7886056/diff/1/tools/clang/TaskToBind/TaskToBi...
tools/clang/TaskToBind/TaskToBind.cpp:51: llvm::errs() << "PostTask with less
than 2 args?!  Inconceivable!";
You can emit diagnostics instead, that way the compiler will report problems as
errors. See ChromeClassTester::emitWarning() in tools/clang/plugins

http://codereview.chromium.org/7886056/diff/1/tools/clang/TaskToBind/TaskToBi...
tools/clang/TaskToBind/TaskToBind.cpp:63: if (kPostTaskName ==
method_decl->getNameAsString()) {
Alternatively, you could use the Resolver stuff in
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/v8/tools/gcmole/gcmol...
, look up the actual Decl of the method, and just compare decl pointers. That
feels cleaner, but I think what you have is ok too

http://codereview.chromium.org/7886056/diff/1/tools/clang/TaskToBind/TaskToBi...
tools/clang/TaskToBind/TaskToBind.cpp:69: if
(interesting_classes_.find(classname) != interesting_classes_.end()) {
nit: .count(classname) > 0

http://codereview.chromium.org/7886056/diff/1/tools/clang/TaskToBind/TaskToBi...
tools/clang/TaskToBind/TaskToBind.cpp:77: // Strip implicit casts.
Instead:

post_task_arg = post_task_arg->IgnoreImpCasts(). Look at
third_party/llvm/tools/clang/include/clang/Expr.h (it will only be there if you
ever ran tools/clang/scripts/update.sh --force-local-build): Maybe you really
want IgnoreImplicit(), IgnoreParenCasts(), or IgnoreConversionOperator()?

http://codereview.chromium.org/7886056/diff/1/tools/clang/TaskToBind/TaskToBi...
tools/clang/TaskToBind/TaskToBind.cpp:140: end = translation_unit->decls_end();
it != end; ++it) {
You could check

  context.getSourceManager()->isFromMainFile((*it)->getLocation())

here and skip everything that isn't.

[awong, 2011-09-16 02:50:41]

For the folks following along at home, draft #2.

This is mainly just cleaning things up.

Next steps:
  1) specify output directory via arguments
  2) Invoke this thing via BeginInvocation()
  3) Add more transforms.
  4) specify transforms via arguments

http://codereview.chromium.org/7886056/diff/1/tools/clang/TaskToBind/README.txt
File tools/clang/TaskToBind/README.txt (right):

http://codereview.chromium.org/7886056/diff/1/tools/clang/TaskToBind/README.t...
tools/clang/TaskToBind/README.txt:9: $ clang -cc1 -load
../../Debug+Asserts/lib/libPrintFunctionNames.so -plugin print-fns
some-input-file.c
On 2011/09/15 02:50:03, Nico wrote:
> print-fns?

Done.

http://codereview.chromium.org/7886056/diff/1/tools/clang/TaskToBind/README.t...
tools/clang/TaskToBind/README.txt:12: $ clang -cc1 -load
../../Debug+Asserts/lib/libPrintFunctionNames.dylib -plugin print-fns
some-input-file.c
On 2011/09/15 02:50:03, Nico wrote:
> Maybe mention
> 
> 
>   GYP_DEFINES='clang=1 clang_load=/abs/path/to/libTaskToBind.dylib
> clang_add_plugin=convert-postask' gclient runhooks
> 
> …then doing a clobber build will run your plugin on _all_ files.

Done.


Though actually this whole file needs to be rewritten after I've gotten all the
semantics worked out.

http://codereview.chromium.org/7886056/diff/1/tools/clang/TaskToBind/TaskToBi...
File tools/clang/TaskToBind/TaskToBind.cpp (right):

http://codereview.chromium.org/7886056/diff/1/tools/clang/TaskToBind/TaskToBi...
tools/clang/TaskToBind/TaskToBind.cpp:51: llvm::errs() << "PostTask with less
than 2 args?!  Inconceivable!";
On 2011/09/15 02:50:03, Nico wrote:
> You can emit diagnostics instead, that way the compiler will report problems
as
> errors. See ChromeClassTester::emitWarning() in tools/clang/plugins

Done.

http://codereview.chromium.org/7886056/diff/1/tools/clang/TaskToBind/TaskToBi...
tools/clang/TaskToBind/TaskToBind.cpp:63: if (kPostTaskName ==
method_decl->getNameAsString()) {
On 2011/09/15 02:50:03, Nico wrote:
> Alternatively, you could use the Resolver stuff in
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/v8/tools/gcmole/gcmol...
> , look up the actual Decl of the method, and just compare decl pointers. That
> feels cleaner, but I think what you have is ok too

Interesting.  That is cleaner. Added TODO.

http://codereview.chromium.org/7886056/diff/1/tools/clang/TaskToBind/TaskToBi...
tools/clang/TaskToBind/TaskToBind.cpp:69: if
(interesting_classes_.find(classname) != interesting_classes_.end()) {
On 2011/09/15 02:50:03, Nico wrote:
> nit: .count(classname) > 0

Done.

http://codereview.chromium.org/7886056/diff/1/tools/clang/TaskToBind/TaskToBi...
tools/clang/TaskToBind/TaskToBind.cpp:77: // Strip implicit casts.
On 2011/09/15 02:50:03, Nico wrote:
> Instead:
> 
> post_task_arg = post_task_arg->IgnoreImpCasts(). Look at
> third_party/llvm/tools/clang/include/clang/Expr.h (it will only be there if
you
> ever ran tools/clang/scripts/update.sh --force-local-build): Maybe you really
> want IgnoreImplicit(), IgnoreParenCasts(), or IgnoreConversionOperator()?

Oh very nice.  Went with IgnoreImplicit()->IgnoreParenCasts().

http://codereview.chromium.org/7886056/diff/1/tools/clang/TaskToBind/TaskToBi...
tools/clang/TaskToBind/TaskToBind.cpp:140: end = translation_unit->decls_end();
it != end; ++it) {
On 2011/09/15 02:50:03, Nico wrote:
> You could check
> 
>   context.getSourceManager()->isFromMainFile((*it)->getLocation())
> 
> here and skip everything that isn't.

Added a note.  I think I'm going to try to make this migrate headers too though,
which would require this...

[Nico, 2011-09-16 03:00:54]

:-)

http://codereview.chromium.org/7886056/diff/6001/tools/clang/BindMigrate/Bind...
File tools/clang/BindMigrate/BindMigrateAction.cpp (right):

http://codereview.chromium.org/7886056/diff/6001/tools/clang/BindMigrate/Bind...
tools/clang/BindMigrate/BindMigrateAction.cpp:56: arcmt_hack::FileRemapper
remapper_;
:-)

If you find that any parts of the ARC stuff are generally useful, maybe I can
get them moved to Rewrite upstream.

http://codereview.chromium.org/7886056/diff/6001/tools/clang/BindMigrate/Bind...
File tools/clang/BindMigrate/BindMigrateConsumer.cpp (right):

http://codereview.chromium.org/7886056/diff/6001/tools/clang/BindMigrate/Bind...
tools/clang/BindMigrate/BindMigrateConsumer.cpp:93: // because there's no such
thing as EndInvocation().
HandleTranslationUnit() is called only after everything's done. Maybe I'm
misunderstanding your comment.

http://codereview.chromium.org/7886056/diff/6001/tools/clang/BindMigrate/Tran...
File tools/clang/BindMigrate/TransformPostTask.cpp (right):

http://codereview.chromium.org/7886056/diff/6001/tools/clang/BindMigrate/Tran...
tools/clang/BindMigrate/TransformPostTask.cpp:44: if (kPostTaskName ==
method_decl->getNameAsString()) {
Nit: with early returns, this gets less nested. It's also recommended llvm
style.

(i.e. 

if (kPostTaskName != method_decl->getNameAsString())
  return false;
string classname = method_decl->getThisType(*context_)
    .getBaseTypeIdentifier()->getName();
return !interesting_classes_.empty();

)

[awong, 2011-09-16 10:24:32]

http://codereview.chromium.org/7886056/diff/6001/tools/clang/BindMigrate/Bind...
File tools/clang/BindMigrate/BindMigrateAction.cpp (right):

http://codereview.chromium.org/7886056/diff/6001/tools/clang/BindMigrate/Bind...
tools/clang/BindMigrate/BindMigrateAction.cpp:56: arcmt_hack::FileRemapper
remapper_;
On 2011/09/16 03:00:54, Nico wrote:
> :-)
> 
> If you find that any parts of the ARC stuff are generally useful, maybe I can
> get them moved to Rewrite upstream.

FileRemapper is generally useful.  So is the rewrite transaction stuff I think. 
Really, the whole ARCMigrate framework could be generalized into something that
runs a set of transforms, where transforms are just AST Visitors.

As is though, FileRemapper isn't quite good enough.  I had to add an extra
function to dump the output into a new output directory.

http://codereview.chromium.org/7886056/diff/6001/tools/clang/BindMigrate/Bind...
File tools/clang/BindMigrate/BindMigrateConsumer.cpp (right):

http://codereview.chromium.org/7886056/diff/6001/tools/clang/BindMigrate/Bind...
tools/clang/BindMigrate/BindMigrateConsumer.cpp:93: // because there's no such
thing as EndInvocation().
On 2011/09/16 03:00:54, Nico wrote:
> HandleTranslationUnit() is called only after everything's done. Maybe I'm
> misunderstanding your comment.

I misunderstood BeginInvocation.

What I was trying to say is that HandleTranslationUnit gets called after each
translation unit is completed...but if you list multiple .cc files (eg., clang++
a.cc b.cc) this will be called once per file.

Ideally, I'd like to be able to say something like remapper.flushToDisk() on
each TranslationUnit which will create a temporary file for each modified file. 


Then at the end of a compiler invocation, either commit the temporary files into
a new directory, or overwrite the originals.  But to do this, I need something
that is run after all translation units are processed.

ARCMigrate solves this a different way...which may actually be nicer.

They actually invoke the compiler 2 times per file inside the BeginInvocation
step before the "real" compilation.

The first time sanity checks that the file parses at all as valid C++.  The
second time is where they actually do the migration.  Then they return out of
BeginInvocation, and allow Clang to try and compile the migrated files.

During the first two compiles it looks like they capture and discard most
Diagnostics by default.  I *think* this lets them deal with partially migrated
code when doing an in place migration.

For BindMigrate, I'm not sure if it's worth bothering with the multiple
compilation steps.  The effect is we'd compile the old source files...but maybe
we don't care for now.

http://codereview.chromium.org/7886056/diff/6001/tools/clang/BindMigrate/Tran...
File tools/clang/BindMigrate/TransformPostTask.cpp (right):

http://codereview.chromium.org/7886056/diff/6001/tools/clang/BindMigrate/Tran...
tools/clang/BindMigrate/TransformPostTask.cpp:44: if (kPostTaskName ==
method_decl->getNameAsString()) {
On 2011/09/16 03:00:54, Nico wrote:
> Nit: with early returns, this gets less nested. It's also recommended llvm
> style.
> 
> (i.e. 
> 
> if (kPostTaskName != method_decl->getNameAsString())
>   return false;
> string classname = method_decl->getThisType(*context_)
>     .getBaseTypeIdentifier()->getName();
> return !interesting_classes_.empty();
> 
> )

Done.

[awong, 2011-09-22 23:01:44]

If I keep at this, I'm going to have a generalized tool that I could try to
upstream into llvm. :-/

This is ready for a real review now.

There is 1 known bug, and 1 wanted feature.

Bug: I can't go from the TemplateArgument in OldCallbackTransform.cpp back to a
source location.  Thus, I'm relying on the pretty printer to regenerate the name
of the template argument.

The problem with this is that it can do weird formatting and insert unnecessary
namespaces.  If you have any ideas about how I can start from either the Stmt,
or the Decl, and get the Start/End Location for each individual template
parameter, that'd be awesome.

The feature I'm working on now is allowing you to restrict the run by type.

Will see how much patience I have :)