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

[Daniel Jasper]

================
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(),
----------------
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.

================
Comment at: tools/clang-check/ClangCheck.cpp:107
@@ +106,3 @@
+public:
+  virtual bool BeginSourceFileAction(clang::CompilerInstance& CI, StringRef Filename) {
+    FixItOpts.reset(new FixItOptions);
----------------
Manuel Klimek wrote:
> 80 columns.
Done.

================
Comment at: tools/clang-check/ClangCheck.cpp:93-103
@@ +92,13 @@
+
+class FixItRewriter : public clang::FixItRewriter {
+public:
+  FixItRewriter(clang::DiagnosticsEngine& Diags,
+                clang::SourceManager& SourceMgr,
+                const clang::LangOptions& LangOpts,
+                clang::FixItOptions* FixItOpts)
+      : clang::FixItRewriter(Diags, SourceMgr, LangOpts, FixItOpts) {
+  }
+
+  virtual bool IncludeInDiagnosticCounts() const { return false; }
+};
+
----------------
Chandler Carruth wrote:
> This class needs a comment about why the existing one won't suffice...
Done.

================
Comment at: tools/clang-check/ClangCheck.cpp:65-70
@@ -60,1 +64,8 @@
 
+static cl::opt<bool> Fixit(
+    "fixit",
+    cl::desc(Options->getOptionHelpText(options::OPT_fixit)));
+static cl::opt<bool> FixWhatYouCan(
+    "fix-what-you-can",
+    cl::desc(Options->getOptionHelpText(options::OPT_fix_what_you_can)));
+
----------------
Chandler Carruth wrote:
> By re-using the existing Clang options, you've made a bit of a surprising change here:
> 
> The original 'clang-fixit' tool behaved as if passing both '-fixit' and '-fix-what-you-can' to Clang. The rationale was that because it is a separate explicit step, it is much less problematic to apply partial fixes.
> 
> I think that logic continues to hold for a 'clang-check' variant. It seems a bit strange to require the extra flag here. =/ Oh well.
> 
> Maybe in the docs somewhere, suggest creating a shell alias for 'clang-fixit' -> 'clang-check -fixit -fix-what-you-can' ?
I agree that it is not ideal, but I would vote for being as consistent with the bare clang as possible. I will update the docs once this is submitted.

================
Comment at: tools/clang-check/ClangCheck.cpp:135
@@ -78,3 +134,3 @@
 int main(int argc, const char **argv) {
   clang_check::ClangCheckActionFactory Factory;
   CommonOptionsParser OptionsParser(argc, argv);
----------------
Manuel Klimek wrote:
> I'd change the scope to after if (Fixit).
Done.


http://llvm-reviews.chandlerc.com/D51