[cfe-commits] [PATCH] Implements support to run standalone tools

[Hal Finkel]

I think that the comments look better; someone else will need to comment
on the actual code ;)

To make the comments even better, IMHO, take the 'Background' paragraph
that you wrote for your post to the CMake developer's list
(http://www.cmake.org/pipermail/cmake-developers/2011-January/000998.html) and put that somewhere in this patch (docs or comment at the top of one  of the files, etc.) This makes it very clear why the functionality is useful.

One code comment: I think that you should make the json file name
overridable somehow. Can you make it a command-line parameter?

 -Hal

On Mon, 2012-01-30 at 20:49 +0100, Manuel Klimek wrote:
> Ping + a re-based version of the patch.
> 
> On Wed, Jan 25, 2012 at 3:59 PM, Manuel Klimek <klimek at google.com> wrote:
> > On Tue, Jan 24, 2012 at 4:43 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> >> On Tue, 2012-01-24 at 11:03 +0100, Manuel Klimek wrote:
> >>> The attached patch adds support to run clang tools (FrontendActions)
> >>> as standalone tools, or repeatedly in-memory in a process.
> >>> This is useful for unit testing, map-reduce-style applications, source
> >>> transformation daemons, and command line tools.
> >>
> >> I am also interested in having this kind of functionality. A few quick
> >> comments:
> >>
> >> 1. The coding standards say that function names should begin with a
> >> lower-case letter.
> >
> > Done. I jumped on the opportunity to dogfood refactoring support in
> > our current tooling branch and wrote a script that changed all
> > incorrectly named functions automatically (and created a sed-script to
> > post-produce comment changes, which made me notice a bug in a
> > comment).
> >
> >> 2. The comments contain several references to CMake; what, if anything,
> >> in this patch is tied to CMake, or designed to be compatible with CMake?
> >>
> >> 2b.
> >>
> >>
> >>> +/// \param JsonDatabase A JSON formatted list of compile commands.
> >>> This lookup
> >>> +/// command supports only a subset of the JSON standard as written by
> >>> CMake.
> >>>
> >>
> >> Please be more verbose here. What is not supported?
> >>
> >> Generally, I think that it would be helpful for you to provide a
> >> paragraph or two explaining how this extension is to be used, what kind
> >> of things can be specified in JSON inputs, how this ties into CMake (or
> >> not), etc. with a few small examples. Some of this can be gleaned from
> >> the test case, but some nicely-formatted text (without all of the
> >> escaping) would, IMHO, be earlier to read.
> >
> > Hopefully better expressed now. Please let me know if you want more /
> > different details.
> >
> > Thanks a lot for the review!
> > /Manuel
> >
> >>
> >>  -Hal
> >>
> >>>
> >>> Cheers,
> >>> /Manuel
> >>>
> >>> Rietveld link:
> >>> http://codereview.appspot.com/5570054/
> >>> _______________________________________________
> >>> cfe-commits mailing list
> >>> cfe-commits at cs.uiuc.edu
> >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >>
> >> --
> >> Hal Finkel
> >> Postdoctoral Appointee
> >> Leadership Computing Facility
> >> Argonne National Laboratory
> >>

-- 
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory