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

Chandler Carruth reviews at llvm-reviews.chandlerc.com
Tue Oct 2 13:14:41 PDT 2012


  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. ;]


================
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



More information about the cfe-commits mailing list