[clang-tidy] Move -extra-arg handling to CommonOptionsProvider

[alexfh]

Summary

Handle -extra-arg and -extra-arg-before options in the
CommonOptionsProvider so they can be used in all clang tools. Adjust arguments
in a CompilationDatabase wrapper instead of adding ArgumentsAdjuster to the
tool.

[klimek]

lib/Tooling/CommonOptionsParser.cpp

138–139

It seems to me like we'd want either this to take an ArgumentAdjuster instead of hard-coding to the args-before / args-after case?

[alexfh]

lib/Tooling/CommonOptionsParser.cpp

138–139

This would make sense, if we make this compilation database wrapper public. But for this specific case the total amount of code is not larger than what would be needed for the ArgumentsAdjuster-based variant.

[klimek]

lib/Tooling/CommonOptionsParser.cpp

138–139

Well, if the code is not much larger, I'd go with the variant that fits the names better... Or do you have a specific reason for not handing in an argument adjuster? To me it was just unexpected when reading the code...

[alexfh]

lib/Tooling/CommonOptionsParser.cpp

138–139

I'd like to see ArgumentsAdjuster replaced with an appropriate std::function eventually. As for the name of the compilation database wrapper, I'd better change it to something closer to the implementation, than vice versa. How about ExtraArgumentsInserterCompilations? Or CompilationsWithExtraArguments?

[klimek]

lib/Tooling/CommonOptionsParser.cpp

138–139

I'm not convinced that renaming is better - can you provide an argument on why you prefer the current implementation (you said the more composable implementation would be ~ the same length).

[alexfh]

Make ArgumentsAdjustingCompilations more generic by using ArgumentsAdjusters.

[alexfh]

The previous version looks marginally better to me due to slightly less
code, but this one turned out to be better than I expected. Which one do
you prefer?

[klimek]

LG. I prefer the current version.

lib/Tooling/CommonOptionsParser.cpp

82

Do we also have to adjust here?

[alexfh]

lib/Tooling/CommonOptionsParser.cpp

82

I think, it's used only in tests, but I see no reason not to adjust the commands here.

[alexfh]

Adjust arguments in getAllCompileCommands().

include/clang/Tooling/ArgumentsAdjusters.h

class ArgumentsAdjuster {
   virtual void anchor();
 public:
   /// \brief Returns adjusted command line arguments.
   ///
   /// \param Args Input sequence of command line arguments.
   ///
   /// \returns Modified sequence of command line arguments.
   virtual CommandLineArguments Adjust(const CommandLineArguments &Args) = 0;
-  virtual ~ArgumentsAdjuster() {
-  }
+  virtual ~ArgumentsAdjuster() {}
 };
 
 /// \brief Syntax check only command line adjuster.
 ///
 /// This class implements ArgumentsAdjuster interface and converts input
 /// command line arguments to the "syntax check only" variant.
 class ClangSyntaxOnlyAdjuster : public ArgumentsAdjuster {
   CommandLineArguments Adjust(const CommandLineArguments &Args) override;
 };
 
 /// \brief An argument adjuster which removes output-related command line
 /// arguments.
 class ClangStripOutputAdjuster : public ArgumentsAdjuster {
   CommandLineArguments Adjust(const CommandLineArguments &Args) override;
 };
 
+class InsertArgumentAdjuster : public ArgumentsAdjuster {
+public:
+  enum Position { BEGIN, END };
+
+  InsertArgumentAdjuster(const CommandLineArguments &Extra, Position Pos)
+      : Extra(Extra), Pos(Pos) {}
+
+  InsertArgumentAdjuster(const char *Extra, Position Pos)
+      : Extra(1, std::string(Extra)), Pos(Pos) {}
+
+  CommandLineArguments Adjust(const CommandLineArguments &Args) override;
+
+private:
+  const CommandLineArguments Extra;
+  const Position Pos;
+};
 } // end namespace tooling
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLING_ARGUMENTSADJUSTERS_H
 

include/clang/Tooling/CommonOptionsParser.h

std::vector<std::string> getSourcePathList() {
     return SourcePathList;
   }
 
   static const char *const HelpMessage;
 
 private:
   std::unique_ptr<CompilationDatabase> Compilations;
   std::vector<std::string> SourcePathList;
+  std::vector<std::string> ExtraArgsBefore;
+  std::vector<std::string> ExtraArgsAfter;
 };
 
 }  // namespace tooling
 }  // namespace clang
 
 #endif  // LLVM_TOOLS_CLANG_INCLUDE_CLANG_TOOLING_COMMONOPTIONSPARSER_H

lib/Tooling/ArgumentsAdjusters.cpp

if(Arg == "-o") {
       // Output is specified as -o foo. Skip the next argument also.
       ++i;
     }
     // Else, the output is specified as -ofoo. Just do nothing.
   }
   return AdjustedArgs;
 }
 
+CommandLineArguments
+InsertArgumentAdjuster::Adjust(const CommandLineArguments &Args) {
+  CommandLineArguments Return(Args);
+
+  CommandLineArguments::iterator I;
+  if (Pos == END) {
+    I = Return.end();
+  } else {
+    I = Return.begin();
+    ++I; // To leave the program name in place
+  }
+
+  Return.insert(I, Extra.begin(), Extra.end());
+  return Return;
+}
+
 } // end namespace tooling
 } // end namespace clang
 

lib/Tooling/CommonOptionsParser.cpp

 //
 //  This class uses the Clang Tooling infrastructure, see
 //    http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 //  for details on setting it up with LLVM source tree.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/CommandLine.h"
+#include "clang/Tooling/ArgumentsAdjusters.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Tooling.h"
 
 using namespace clang::tooling;
 using namespace llvm;
 
 const char *const CommonOptionsParser::HelpMessage =
     "\n"
const char *const CommonOptionsParser::HelpMessage =
     "\tabsolute, it needs to point into CMake's source tree. If the path is\n"
     "\trelative, the current working directory needs to be in the CMake\n"
     "\tsource tree and the file must be in a subdirectory of the current\n"
     "\tworking directory. \"./\" prefixes in the relative files will be\n"
     "\tautomatically removed, but the rest of a relative path must be a\n"
     "\tsuffix of a path in the compile command database.\n"
     "\n";
 
+class ArgumentsAdjustingCompilations : public CompilationDatabase {
+public:
+  ArgumentsAdjustingCompilations(
+      std::unique_ptr<CompilationDatabase> Compilations)
+      : Compilations(std::move(Compilations)) {}
+
+  void appendArgumentsAdjuster(std::unique_ptr<ArgumentsAdjuster> Adjuster) {
+    Adjusters.push_back(std::move(Adjuster));
+  }
+
+  std::vector<CompileCommand>
+  getCompileCommands(StringRef FilePath) const override {
+    std::vector<CompileCommand> Commands =
+        Compilations->getCompileCommands(FilePath);
+    for (CompileCommand &Command : Commands)
+      for (const auto &Adjuster : Adjusters)
+        Command.CommandLine = Adjuster->Adjust(Command.CommandLine);
+    return Commands;
+  }
+
+  std::vector<std::string> getAllFiles() const override {
+    return Compilations->getAllFiles();
+  }
+
+  std::vector<CompileCommand> getAllCompileCommands() const override {
+    return Compilations->getAllCompileCommands();
+  }
+
+private:
+  std::unique_ptr<CompilationDatabase> Compilations;
+  std::vector<std::unique_ptr<ArgumentsAdjuster>> Adjusters;
+};
+
 CommonOptionsParser::CommonOptionsParser(int &argc, const char **argv,
                                          cl::OptionCategory &Category,
                                          const char *Overview) {
   static cl::opt<bool> Help("h", cl::desc("Alias for -help"), cl::Hidden);
 
   static cl::opt<std::string> BuildPath("p", cl::desc("Build path"),
                                         cl::Optional, cl::cat(Category));
 
   static cl::list<std::string> SourcePaths(
       cl::Positional, cl::desc("<source0> [... <sourceN>]"), cl::OneOrMore,
       cl::cat(Category));
 
+  static cl::list<std::string> ArgsAfter(
+      "extra-arg",
+      cl::desc("Additional argument to append to the compiler command line"),
+      cl::cat(Category));
+
+  static cl::list<std::string> ArgsBefore(
+      "extra-arg-before",
+      cl::desc("Additional argument to prepend to the compiler command line"),
+      cl::cat(Category));
+
   // Hide unrelated options.
   StringMap<cl::Option*> Options;
   cl::getRegisteredOptions(Options);
   for (StringMap<cl::Option *>::iterator I = Options.begin(), E = Options.end();
        I != E; ++I) {
     if (I->second->Category != &Category && I->first() != "help" &&
         I->first() != "version")
       I->second->setHiddenFlag(cl::ReallyHidden);
if (!BuildPath.empty()) {
           CompilationDatabase::autoDetectFromDirectory(BuildPath, ErrorMessage);
     } else {
       Compilations = CompilationDatabase::autoDetectFromSource(SourcePaths[0],
                                                                ErrorMessage);
     }
     if (!Compilations)
       llvm::report_fatal_error(ErrorMessage);
   }
+  auto AdjustingCompilations =
+      llvm::make_unique<ArgumentsAdjustingCompilations>(
+          std::move(Compilations));
+  AdjustingCompilations->appendArgumentsAdjuster(
+      llvm::make_unique<InsertArgumentAdjuster>(ArgsBefore,
+                                                InsertArgumentAdjuster::BEGIN));
+  AdjustingCompilations->appendArgumentsAdjuster(
+      llvm::make_unique<InsertArgumentAdjuster>(ArgsAfter,
+                                                InsertArgumentAdjuster::END));
+  Compilations = std::move(AdjustingCompilations);
 }

tools/clang-check/ClangCheck.cpp

 static cl::opt<bool>
 Fixit("fixit", cl::desc(Options->getOptionHelpText(options::OPT_fixit)),
       cl::cat(ClangCheckCategory));
 static cl::opt<bool> FixWhatYouCan(
     "fix-what-you-can",
     cl::desc(Options->getOptionHelpText(options::OPT_fix_what_you_can)),
     cl::cat(ClangCheckCategory));
 
-static cl::list<std::string> ArgsAfter(
-    "extra-arg",
-    cl::desc("Additional argument to append to the compiler command line"),
-    cl::cat(ClangCheckCategory));
-static cl::list<std::string> ArgsBefore(
-    "extra-arg-before",
-    cl::desc("Additional argument to prepend to the compiler command line"),
-    cl::cat(ClangCheckCategory));
-
 namespace {
 
 // FIXME: Move FixItRewriteInPlace from lib/Rewrite/Frontend/FrontendActions.cpp
 // into a header file and reuse that.
 class FixItOptions : public clang::FixItOptions {
 public:
   FixItOptions() {
     FixWhatYouCan = ::FixWhatYouCan;
bool BeginSourceFileAction(clang::CompilerInstance& CI,
                              StringRef Filename) override {
     FixItOpts.reset(new FixItOptions);
     Rewriter.reset(new FixItRewriter(CI.getDiagnostics(), CI.getSourceManager(),
                                      CI.getLangOpts(), FixItOpts.get()));
     return true;
   }
 };
 
-class InsertAdjuster: public clang::tooling::ArgumentsAdjuster {
-public:
-  enum Position { BEGIN, END };
-
-  InsertAdjuster(const CommandLineArguments &Extra, Position Pos)
-    : Extra(Extra), Pos(Pos) {
-  }
-
-  InsertAdjuster(const char *Extra, Position Pos)
-    : Extra(1, std::string(Extra)), Pos(Pos) {
-  }
-
-  virtual CommandLineArguments
-  Adjust(const CommandLineArguments &Args) override {
-    CommandLineArguments Return(Args);
-
-    CommandLineArguments::iterator I;
-    if (Pos == END) {
-      I = Return.end();
-    } else {
-      I = Return.begin();
-      ++I; // To leave the program name in place
-    }
-
-    Return.insert(I, Extra.begin(), Extra.end());
-    return Return;
-  }
-
-private:
-  const CommandLineArguments Extra;
-  const Position Pos;
-};
-
-} // 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:
   std::unique_ptr<clang::ASTConsumer> newASTConsumer() {
     if (ASTList)
       return clang::CreateASTDeclNodeLister();
     if (ASTDump)
-      return clang::CreateASTDumper(ASTDumpFilter, /*DumpDecls*/ true,
-                                    /*DumpLookups*/ false);
+      return clang::CreateASTDumper(ASTDumpFilter, /*DumpDecls=*/true,
+                                    /*DumpLookups=*/false);
     if (ASTPrint)
       return clang::CreateASTPrinter(&llvm::outs(), ASTDumpFilter);
     return llvm::make_unique<clang::ASTConsumer>();
   }
 };
-}
+
+} // namespace
 
 int main(int argc, const char **argv) {
   llvm::sys::PrintStackTraceOnErrorSignal();
   CommonOptionsParser OptionsParser(argc, argv, ClangCheckCategory);
   ClangTool Tool(OptionsParser.getCompilations(),
                  OptionsParser.getSourcePathList());
 
   // Clear adjusters because -fsyntax-only is inserted by the default chain.
   Tool.clearArgumentsAdjusters();
   Tool.appendArgumentsAdjuster(new ClangStripOutputAdjuster());
-  if (ArgsAfter.size() > 0) {
-    Tool.appendArgumentsAdjuster(new InsertAdjuster(ArgsAfter,
-          InsertAdjuster::END));
-  }
-  if (ArgsBefore.size() > 0) {
-    Tool.appendArgumentsAdjuster(new InsertAdjuster(ArgsBefore,
-          InsertAdjuster::BEGIN));
-  }
 
   // Running the analyzer requires --analyze. Other modes can work with the
   // -fsyntax-only option.
-  Tool.appendArgumentsAdjuster(new InsertAdjuster(
-        Analyze ? "--analyze" : "-fsyntax-only", InsertAdjuster::BEGIN));
+  Tool.appendArgumentsAdjuster(new InsertArgumentAdjuster(
+      Analyze ? "--analyze" : "-fsyntax-only", InsertArgumentAdjuster::BEGIN));
 
-  clang_check::ClangCheckActionFactory CheckFactory;
+  ClangCheckActionFactory CheckFactory;
   std::unique_ptr<FrontendActionFactory> FrontendFactory;
 
   // Choose the correct factory based on the selected mode.
   if (Analyze)
     FrontendFactory = newFrontendActionFactory<clang::ento::AnalysisAction>();
   else if (Fixit)
     FrontendFactory = newFrontendActionFactory<FixItAction>();
   else
     FrontendFactory = newFrontendActionFactory(&CheckFactory);
 
   return Tool.run(FrontendFactory.get());
 }