Add -fixit option to clang-check

[djasper]

Summary

As opposed to the clang-fixit tool described on http://clang.llvm.org/docs/ClangTools.html, this adds -fixit option to clang-check. Thus, clang-check can become a general-purpose tool to run clang capitalizing on the info stored in a compilation database.

[doug.gregor]

General comment: why do we need a separate tool at all? Can't this simply be a "-fixit" option to the perhaps-to-be-renamed clang-check?

[djasper]

I had thought about that and could find any strong arguments for one or the other. Chandler: I think you have authored the webpage mentioning clang-fixit as a separate tool. Any reason why it should be a separate tool?

[djasper]

... could not find ...

[silvas]

From the description on the page: "It is designed to explore alternative interfaces for applying fix-it hints, including automatic application, prompting users with options for different fixes, etc.", it seems like the tool is meant to be a bit more involved.

[klimek]

tools/clang-check/ClangCheck.cpp

107

80 columns.

140–141

I'd change the scope to after if (Fixit).

[chandlerc]

Bleh. Way too much boiler plate here. =/

tools/clang-check/ClangCheck.cpp

65–70

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' ?

93–103

This class needs a comment about why the existing one won't suffice...

108

Rather than a whole separate class above, why not just clobber the one option here?

[djasper]

tools/clang-check/ClangCheck.cpp

65–70

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.

93–103

Done.

107

Done.

108

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.

140–141

Done.

[chandlerc]

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

tools/clang-check/ClangCheck.cpp

108

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.

110

Probably should toss a comment on this class as well to keep a record of some of our discussion about why it is needed...

[djasper]

Comment added and submitted as r165110. Cleanup CL as well as CL changing documentation will follow.

tools/clang-check/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   ${LLVM_TARGETS_TO_BUILD}
   asmparser
   support
   mc
   )
 
 add_clang_executable(clang-check
   ClangCheck.cpp
   )
 
 target_link_libraries(clang-check
   clangTooling
   clangBasic
+  clangRewriteFrontend
   )
 
 install(TARGETS clang-check
   RUNTIME DESTINATION bin)

tools/clang-check/ClangCheck.cpp

 //===--- tools/clang-check/ClangCheck.cpp - Clang check tool --------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
-//  This file implements a clang-check tool that runs the
-//  clang::SyntaxOnlyAction over a number of translation units.
+//  This file implements a clang-check tool that runs clang based on the info
+//  stored in a compilation database.
 //
 //  This tool uses the Clang Tooling infrastructure, see
 //    http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 //  for details on setting it up with LLVM source tree.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/AST/ASTConsumer.h"
 #include "clang/Driver/OptTable.h"
 #include "clang/Driver/Options.h"
 #include "clang/Frontend/ASTConsumers.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Rewrite/Frontend/FixItRewriter.h"
+#include "clang/Rewrite/Frontend/FrontendActions.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Path.h"
 
 using namespace clang::driver;
 using namespace clang::tooling;
 using namespace llvm;
 
 static cl::extrahelp CommonHelp(CommonOptionsParser::HelpMessage);
 static cl::extrahelp MoreHelp(
     "\tFor example, to run clang-check on all files in a subtree of the\n"
static cl::opt<bool> ASTList(
     cl::desc(Options->getOptionHelpText(options::OPT_ast_list)));
 static cl::opt<bool> ASTPrint(
     "ast-print",
     cl::desc(Options->getOptionHelpText(options::OPT_ast_print)));
 static cl::opt<std::string> ASTDumpFilter(
     "ast-dump-filter",
     cl::desc(Options->getOptionHelpText(options::OPT_ast_dump_filter)));
 
+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)));
+
+namespace {
+
+class FixItOptions : public clang::FixItOptions {
+public:
+  FixItOptions() {
+    FixWhatYouCan = ::FixWhatYouCan;
+  }
+
+  std::string RewriteFilename(const std::string& filename, int &fd) {
+    assert(llvm::sys::path::is_absolute(filename) &&
+           "clang-fixit expects absolute paths only.");
+
+    // We don't need to do permission checking here since clang will diagnose
+    // any I/O errors itself.
+
+    fd = -1;  // No file descriptor for file.
+
+    return filename;
+  }
+};
+
+/// \brief Subclasses \c clang::FixItRewriter to not count fixed errors/warnings
+/// in the final error counts.
+///
+/// This has the side-effect that clang-check -fixit exits with code 0 on
+/// successfully fixing all errors.
+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; }
+};
+
+class FixItAction : public clang::FixItAction {
+public:
+  virtual bool BeginSourceFileAction(clang::CompilerInstance& CI,
+                                     StringRef Filename) {
+    FixItOpts.reset(new FixItOptions);
+    Rewriter.reset(new FixItRewriter(CI.getDiagnostics(), CI.getSourceManager(),
+                                     CI.getLangOpts(), FixItOpts.get()));
+    return true;
+  }
+};
+
+} // namespace
+
 // Anonymous namespace here causes problems with gcc <= 4.4 on MacOS 10.6.
 // "Non-global symbol: ... can't be a weak_definition"
 namespace clang_check {
 class ClangCheckActionFactory {
 public:
   clang::ASTConsumer *newASTConsumer() {
     if (ASTList)
       return clang::CreateASTDeclNodeLister();
     if (ASTDump)
       return clang::CreateASTDumper(ASTDumpFilter);
     if (ASTPrint)
       return clang::CreateASTPrinter(&llvm::outs(), ASTDumpFilter);
     return new clang::ASTConsumer();
   }
 };
 }
 
 int main(int argc, const char **argv) {
-  clang_check::ClangCheckActionFactory Factory;
   CommonOptionsParser OptionsParser(argc, argv);
   ClangTool Tool(OptionsParser.GetCompilations(),
                  OptionsParser.GetSourcePathList());
+  if (Fixit)
+    return Tool.run(newFrontendActionFactory<FixItAction>());
+  clang_check::ClangCheckActionFactory Factory;
   return Tool.run(newFrontendActionFactory(&Factory));
 }

tools/clang-check/Makefile

 
 # No plugins, optimize startup time.
 TOOL_NO_EXPORTS = 1
 
 include $(CLANG_LEVEL)/../../Makefile.config
 LINK_COMPONENTS := $(TARGETS_TO_BUILD) asmparser support mc
 USEDLIBS = clangFrontend.a clangSerialization.a clangDriver.a \
            clangTooling.a clangParse.a clangSema.a clangAnalysis.a \
-           clangEdit.a clangAST.a clangLex.a clangBasic.a
+           clangEdit.a clangAST.a clangLex.a clangBasic.a \
+           clangRewriteFrontend.a
 
 include $(CLANG_LEVEL)/Makefile