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

Manuel Klimek klimek at google.com
Tue Jan 31 03:53:16 CST 2012


On Tue, Jan 31, 2012 at 12:44 AM, Hal Finkel <hfinkel at anl.gov> wrote:
> 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.

This is a great idea. Done.

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

I'm planning to do some rework on the command line handling anyway
(currently it's really hard to add parameters to your tool, and I'd
like to use standard llvm command line handling somehow), but I'd
really like to do that iteratively. If you think that this is
important enough to block the patch going in, let me know.

Cheers,
/Manuel

>  -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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tooling.patch
Type: text/x-patch
Size: 42831 bytes
Desc: not available
Url : http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20120131/f51e79b4/attachment-0001.bin 


More information about the cfe-commits mailing list