On Tue, Oct 2, 2012 at 1:14 PM, Chandler Carruth
<[email protected]> wrote:
>
> LGTM
>
> For what its worth, I think the amount of code we have added here clearly
> demonstrates why this should be a separate tool... but I'll leave that debate
> for another day. ;]
Feel free to argue with Doug :) He's clearly the one feeling the most
strongly about this; I mainly want that feature in the hands of our
users ...
> ================
> Comment at: tools/clang-check/ClangCheck.cpp:108
> @@ +107,3 @@
> + virtual bool BeginSourceFileAction(clang::CompilerInstance& CI, StringRef
> Filename) {
> + FixItOpts.reset(new FixItOptions);
> + Rewriter.reset(new FixItRewriter(CI.getDiagnostics(),
> CI.getSourceManager(),
> ----------------
> Daniel Jasper wrote:
>> Chandler Carruth wrote:
>> > Rather than a whole separate class above, why not just clobber the one
>> > option here?
>> Because all implementations (preferable we would reuse FixItRewriteInPlace)
>> are defined in an unnamed namespace inside
>> lib/Rewrite/Frontend/FrontendActions.cpp. Would you rather, I move those
>> into clang/Rewrite/Frontend/FrontendActions.h?
>>
>> Also, we cannot just call clang::FixItAction::BeginSourceFileAction() as it
>> installs clang::FixItRewriter() which we don't want.
> If we can effectively re-use an implementation, then yes, we should lift it
> into the public header file. It's not clear based on your last comment
> whether we can re-use it.
>
> I'm happy for this to be a follow-up patch though to clean things up.
>
> ================
> Comment at: tools/clang-check/ClangCheck.cpp:110
> @@ +109,3 @@
> +
> +class FixItAction : public clang::FixItAction {
> +public:
> ----------------
> Probably should toss a comment on this class as well to keep a record of some
> of our discussion about why it is needed...
>
>
> http://llvm-reviews.chandlerc.com/D51
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
