[cfe-commits] [PATCH] Add -fixit option to clang-check

[Manuel Klimek]

On Tue, Oct 2, 2012 at 1:14 PM, Chandler Carruth
<reviews at llvm-reviews.chandlerc.com> 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