Add ExtraArgs and ExtraArgsBefore options to enable clang warnings via configuration files.

[alexfh]

Summary

This patch depends on http://reviews.llvm.org/D14191

[alexfh]

Ping.

[alexfh]

Ping.

[djasper]

clang-tidy/ClangTidy.cpp

47

This doesn't seem to be the right place for this #include.

clang-tidy/ClangTidyOptions.h

89–93

Are we sure we are going to need both? Maybe for now only add the one you are actually using (and testing)?

[alexfh]

Addressed review comments.

clang-tidy/ClangTidyOptions.h

89–93

Though it's a less frequently used option, but seems to be good for consistency (with the clang tools' -extra-arg/-extra-arg-before pair of options) and for completeness. I've extended the test to verify both options and their interaction.

[alexfh]

PTAL

[djasper]

I'll let Manuel review this tomorrow. He expressed some concerns.

[klimek]

clang-tidy/ClangTidy.cpp

382

My concern is still with the Filename in the arg callback. I'd like to understand the full use-case better.
From this code it looks like we want to change the ClangTidyContext's file and use the callback for that? That seems rather out of place, but maybe I'm missing something.

[alexfh]

clang-tidy/ClangTidy.cpp

382

We need the filename to retrieve the proper ClangTidyOptions for each file. Context.setCurrentFile is used to specify which file should getOptions return the options for.

[klimek]

clang-tidy/ClangTidy.cpp

382

But the extra-args don't depend on the file; it seems like the filename in the callback is only used to be notified when the file changes?

[alexfh]

clang-tidy/ClangTidy.cpp

382

The thing is that they do depend on the file, because we're implementing their configuration from the config file, and different translation units can have different corresponding configuration files.

[klimek]

clang-tidy/ClangTidy.cpp

382

Ok, now I get it. I completely didn't expect an interface that needs a setCurrentFile so it can getOptions afterwards for that file.
In that case, the Filename is OK, but I suggest changing the ClangTidyOptions to provide getOptions(file) instead.

[alexfh]

Added ClangTidyContext::getOptionsForFile.

[alexfh]

Updated documentation comments.

[alexfh]

PTAL

clang-tidy/ClangTidy.cpp

382

The interface should be clearer, if you think about the possibility of having different options for different files. Also, the documentation on the corresponding methods should help. But adding getOptionsForFile doesn't hurt.

[klimek]

Ok, this patch LG. I'd really like to get rid of the current-file-dependent stuff in ClangTidyOptions though :)

clang-tidy/ClangTidy.cpp

 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Rewrite/Frontend/FixItRewriter.h"
 #include "clang/Rewrite/Frontend/FrontendActions.h"
 #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
 #include "clang/Tooling/Refactoring.h"
 #include "clang/Tooling/ReplacementsYaml.h"
 #include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Signals.h"
 #include <algorithm>
 #include <utility>
 
 using namespace clang::ast_matchers;
 using namespace clang::driver;
 using namespace clang::tooling;
 using namespace llvm;
 
 template class llvm::Registry<clang::tidy::ClangTidyModule>;
 
 namespace clang {
 namespace tidy {
 

 
 ClangTidyStats
 runClangTidy(std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider,
              const tooling::CompilationDatabase &Compilations,
              ArrayRef<std::string> InputFiles,
              std::vector<ClangTidyError> *Errors, ProfileData *Profile) {
   ClangTool Tool(Compilations, InputFiles);
   clang::tidy::ClangTidyContext Context(std::move(OptionsProvider));
+  ArgumentsAdjuster PerFileExtraArgumentsInserter = [&Context](
+      const CommandLineArguments &Args, StringRef Filename) {
+    ClangTidyOptions Opts = Context.getOptionsForFile(Filename);
+    CommandLineArguments AdjustedArgs;
+    if (Opts.ExtraArgsBefore)
+      AdjustedArgs = *Opts.ExtraArgsBefore;
+    AdjustedArgs.insert(AdjustedArgs.begin(), Args.begin(), Args.end());
+    if (Opts.ExtraArgs)
+      AdjustedArgs.insert(AdjustedArgs.end(), Opts.ExtraArgs->begin(),
+                          Opts.ExtraArgs->end());
+    return AdjustedArgs;
+  };
+  Tool.appendArgumentsAdjuster(PerFileExtraArgumentsInserter);
   if (Profile)
     Context.setCheckProfileData(Profile);
 
   ClangTidyDiagnosticConsumer DiagConsumer(Context);
 
   Tool.setDiagnosticConsumer(&DiagConsumer);
 
   class ActionFactory : public FrontendActionFactory {

clang-tidy/ClangTidyDiagnosticConsumer.h

public:
   void setCurrentFile(StringRef File);
 
   /// \brief Returns the main file name of the current translation unit.
   StringRef getCurrentFile() const { return CurrentFile; }
 
   /// \brief Sets ASTContext for the current translation unit.
   void setASTContext(ASTContext *Context);
 
-  /// \brief Gets the language options from the AST context
+  /// \brief Gets the language options from the AST context.
   LangOptions getLangOpts() const { return LangOpts; }
 
   /// \brief Returns the name of the clang-tidy check which produced this
   /// diagnostic ID.
   StringRef getCheckName(unsigned DiagnosticID) const;
 
   /// \brief Returns check filter for the \c CurrentFile.
+  ///
+  /// The \c CurrentFile can be changed using \c setCurrentFile.
   GlobList &getChecksFilter();
 
   /// \brief Returns global options.
   const ClangTidyGlobalOptions &getGlobalOptions() const;
 
   /// \brief Returns options for \c CurrentFile.
+  ///
+  /// The \c CurrentFile can be changed using \c setCurrentFile.
   const ClangTidyOptions &getOptions() const;
 
+  /// \brief Returns options for \c File. Does not change or depend on
+  /// \c CurrentFile.
+  ClangTidyOptions getOptionsForFile(StringRef File) const;
+
   /// \brief Returns \c ClangTidyStats containing issued and ignored diagnostic
   /// counters.
   const ClangTidyStats &getStats() const { return Stats; }
 
   /// \brief Returns all collected errors.
   const std::vector<ClangTidyError> &getErrors() const { return Errors; }
 
   /// \brief Clears collected errors.

clang-tidy/ClangTidyDiagnosticConsumer.cpp

 }
 
 void ClangTidyContext::setSourceManager(SourceManager *SourceMgr) {
   DiagEngine->setSourceManager(SourceMgr);
 }
 
 void ClangTidyContext::setCurrentFile(StringRef File) {
   CurrentFile = File;
-  // Safeguard against options with unset values.
-  CurrentOptions = ClangTidyOptions::getDefaults().mergeWith(
-      OptionsProvider->getOptions(CurrentFile));
+  CurrentOptions = getOptionsForFile(CurrentFile);
   CheckFilter.reset(new GlobList(*getOptions().Checks));
 }
 
 void ClangTidyContext::setASTContext(ASTContext *Context) {
   DiagEngine->SetArgToStringFn(&FormatASTNodeDiagnosticArgument, Context);
   LangOpts = Context->getLangOpts();
 }
 
 const ClangTidyGlobalOptions &ClangTidyContext::getGlobalOptions() const {
   return OptionsProvider->getGlobalOptions();
 }
 
 const ClangTidyOptions &ClangTidyContext::getOptions() const {
   return CurrentOptions;
 }
 
+ClangTidyOptions ClangTidyContext::getOptionsForFile(StringRef File) const {
+  // Merge options on top of getDefaults() as a safeguard against options with
+  // unset values.
+  return ClangTidyOptions::getDefaults().mergeWith(
+      OptionsProvider->getOptions(CurrentFile));
+}
+
 void ClangTidyContext::setCheckProfileData(ProfileData *P) { Profile = P; }
 
 GlobList &ClangTidyContext::getChecksFilter() {
   assert(CheckFilter != nullptr);
   return *CheckFilter;
 }
 
 /// \brief Store a \c ClangTidyError.

clang-tidy/ClangTidyOptions.h

struct ClangTidyOptions {
   /// comments in the relevant check.
   llvm::Optional<std::string> User;
 
   typedef std::pair<std::string, std::string> StringPair;
   typedef std::map<std::string, std::string> OptionMap;
 
   /// \brief Key-value mapping used to store check-specific options.
   OptionMap CheckOptions;
+
+  typedef std::vector<std::string> ArgList;
+
+  /// \brief Add extra compilation arguments to the end of the list.
+  llvm::Optional<ArgList> ExtraArgs;
+
+  /// \brief Add extra compilation arguments to the start of the list.
+  llvm::Optional<ArgList> ExtraArgsBefore;
 };
 
 /// \brief Abstract interface for retrieving various ClangTidy options.
 class ClangTidyOptionsProvider {
 public:
   virtual ~ClangTidyOptionsProvider() {}
 
   /// \brief Returns global options, which are independent of the file.

clang-tidy/ClangTidyOptions.cpp

 #define DEBUG_TYPE "clang-tidy-options"
 
 using clang::tidy::ClangTidyOptions;
 using clang::tidy::FileFilter;
 
 LLVM_YAML_IS_FLOW_SEQUENCE_VECTOR(FileFilter)
 LLVM_YAML_IS_FLOW_SEQUENCE_VECTOR(FileFilter::LineRange)
 LLVM_YAML_IS_SEQUENCE_VECTOR(ClangTidyOptions::StringPair)
+LLVM_YAML_IS_SEQUENCE_VECTOR(std::string)
 
 namespace llvm {
 namespace yaml {
 
 // Map std::pair<int, int> to a JSON array of size 2.
 template <> struct SequenceTraits<FileFilter::LineRange> {
   static size_t size(IO &IO, FileFilter::LineRange &Range) {
     return Range.first == 0 ? 0 : Range.second == 0 ? 1 : 2;
template <> struct MappingTraits<ClangTidyOptions> {
   static void mapping(IO &IO, ClangTidyOptions &Options) {
     MappingNormalization<NOptionMap, ClangTidyOptions::OptionMap> NOpts(
         IO, Options.CheckOptions);
     IO.mapOptional("Checks", Options.Checks);
     IO.mapOptional("HeaderFilterRegex", Options.HeaderFilterRegex);
     IO.mapOptional("AnalyzeTemporaryDtors", Options.AnalyzeTemporaryDtors);
     IO.mapOptional("User", Options.User);
     IO.mapOptional("CheckOptions", NOpts->Options);
+    IO.mapOptional("ExtraArgs", Options.ExtraArgs);
+    IO.mapOptional("ExtraArgsBefore", Options.ExtraArgsBefore);
   }
 };
 
 } // namespace yaml
 } // namespace llvm
 
 namespace clang {
 namespace tidy {
ClangTidyOptions::mergeWith(const ClangTidyOptions &Other) const {
   if (Other.HeaderFilterRegex)
     Result.HeaderFilterRegex = Other.HeaderFilterRegex;
   if (Other.SystemHeaders)
     Result.SystemHeaders = Other.SystemHeaders;
   if (Other.AnalyzeTemporaryDtors)
     Result.AnalyzeTemporaryDtors = Other.AnalyzeTemporaryDtors;
   if (Other.User)
     Result.User = Other.User;
+  if (Other.ExtraArgs)
+    Result.ExtraArgs = Other.ExtraArgs;
+  if (Other.ExtraArgsBefore)
+    Result.ExtraArgsBefore = Other.ExtraArgsBefore;
 
   for (const auto &KeyValue : Other.CheckOptions)
     Result.CheckOptions[KeyValue.first] = KeyValue.second;
 
   return Result;
 }
 
 FileOptionsProvider::FileOptionsProvider(

test/clang-tidy/custom-diagnostics.cpp

+// RUN: clang-tidy -checks='-*,modernize-use-override,clang-diagnostic-shadow,clang-diagnostic-float-conversion' %s -- | count 0
+// RUN: clang-tidy -checks='-*,modernize-use-override,clang-diagnostic-shadow,clang-diagnostic-float-conversion' \
+// RUN:   -config='{ExtraArgs: ["-Wshadow","-Wno-unused-variable"], ExtraArgsBefore: ["-Wno-shadow","-Wfloat-conversion","-Wunused-variable"]}' %s -- \
+// RUN:   | FileCheck -implicit-check-not='{{warning:|error:}}' %s
+
+void f(float x) {
+  int a;
+  { int a; }
+  // CHECK: :[[@LINE-1]]:9: warning: declaration shadows a local variable [clang-diagnostic-shadow]
+  int b = x;
+  // CHECK: :[[@LINE-1]]:11: warning: implicit conversion turns floating-point number into integer: 'float' to 'int' [clang-diagnostic-float-conversion]
+}