[clang-tidy] Add support for custom configuration file names/formats.

[alexfh]

Summary

We're using different clang-tidy frontends (command-line, batch analysis jobs, code review integration), some of which are limited in the choice of configuration format. In order to avoid duplication of configuration information, we need to support the same configuration format in the command-line tool. This patch adds an extension point to make this possible without rewriting FileOptionsProvider.

[djasper]

clang-tidy/ClangTidyOptions.h

129

I think the naming here is quite confusing:

1. ConfigHandler: A single handler
2. ConfigFileHandlers: Multiple handlers
3. ConfigHandlers: Member variable containing the current ConfigFileHandlers

I suggest swappin #2 and #3 or renaming ConfigHandler to ConfigFileHandler.

[alexfh]

Renamed FileOptionsProvider::ConfigHandler to ConfigFileHandler.

[djasper]

Either the patch description or the FileOptionsProvider need a lot more documentation describing the intended usage. This should roughly outline why you might want to parse different configuration files/formats and what you need to implement to do so.

clang-tidy/ClangTidyOptions.cpp

184

Not a strong opinion, but using the concrete type here might help understanding this later.

200–209

I'd move this part of the function into getOptions():
a) The first bit doesn't have to be done for each handler, but just once per directory.
b) We are frequently creating a no_such_file_or_directory error here which we are then explicitly checking later. Using an error code in something that seems like the more frequent use case seems off.

clang-tidy/ClangTidyOptions.h

127

I think a ConfigFileHandler should be a proper class and have (at least) three methods:

• parseFromFile(StringRef file) - Read configuration from file
• configurationAsText() - Print configuration in input-like format
• writeToFile(StringRef) - Save configuration to specific file, ignoring other bits in the file.

Not all of them have to be implemented for all handlers, but that seems like a set of useful functionality.

Also, at least the pair's string needs to be documented.

177

This should document in which order config handlers are tried.

[alexfh]

Added/updated comments, moved the loop over config handlers inside
TryReadConfigFile.

clang-tidy/ClangTidyOptions.cpp

184

Done.

200–209

These are valid points. I've addressed these using the opposite approach: made TryReadConfigFile try all configuration file handlers.

clang-tidy/ClangTidyOptions.h

127

I don't feel that these methods will be immediately useful. We can add them (or alternatives) once we're have the need to surface relevant functionality (e.g. listing supported configuration formats and choosing the format for -dump-config) in the command-line interface.

For now, I suggest leaving it as a pair. I'm adding a comment describing its content and usage.

177

Done.

[alexfh]

Ping.

[djasper]

Looks good.

clang-tidy/ClangTidyOptions.cpp

183

indentation

clang-tidy/ClangTidyOptions.h

144

It seems complicated to have to specify all of GlobalOptions, DefaultOptions and OverrideOptions. Consider adding overloads or something.

nit: indentation ;-)

[alexfh]

clang-tidy/ClangTidyOptions.cpp

183

Done.

clang-tidy/ClangTidyOptions.h

144

For the use case we have, these all parameters are essential. We can add more overloads once we have a specific need.

clang-tidy/ClangTidyOptions.h

 //===--- ClangTidyOptions.h - clang-tidy ------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANG_TIDY_OPTIONS_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANG_TIDY_OPTIONS_H
 
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ErrorOr.h"
+#include <functional>
 #include <map>
 #include <string>
 #include <system_error>
 #include <utility>
 #include <vector>
 
 namespace clang {
 namespace tidy {
public:
   }
 
 private:
   ClangTidyGlobalOptions GlobalOptions;
   ClangTidyOptions DefaultOptions;
 };
 
 /// \brief Implementation of the \c ClangTidyOptionsProvider interface, which
-/// tries to find a .clang-tidy file in the closest parent directory of each
-/// file.
+/// tries to find a configuration file in the closest parent directory of each
+/// source file.
+///
+/// By default, files named ".clang-tidy" will be considered, and the
+/// \c clang::tidy::parseConfiguration function will be used for parsing, but a
+/// custom set of configuration file names and parsing functions can be
+/// specified using the appropriate constructor.
 class FileOptionsProvider : public DefaultOptionsProvider {
 public:
+  // \brief A pair of configuration file base name and a function parsing
+  // configuration from text in the corresponding format.
+  typedef std::pair<std::string, std::function<llvm::ErrorOr<ClangTidyOptions>(
+                                     llvm::StringRef)>> ConfigFileHandler;
+
+  /// \brief Configuration file handlers listed in the order of priority.
+  ///
+  /// Custom configuration file formats can be supported by constructing the
+  /// list of handlers and passing it to the appropriate \c FileOptionsProvider
+  /// constructor. E.g. initialization of a \c FileOptionsProvider with support
+  /// of a custom configuration file format for files named ".my-tidy-config"
+  /// could look similar to this:
+  /// \code
+  /// FileOptionsProvider::ConfigFileHandlers ConfigHandlers;
+  /// ConfigHandlers.emplace_back(".my-tidy-config", parseMyConfigFormat);
+  /// ConfigHandlers.emplace_back(".clang-tidy", parseConfiguration);
+  /// return llvm::make_unique<FileOptionsProvider>(GlobalOptions,
+  ///     DefaultOptions, OverrideOptions, ConfigHandlers);
+  /// \endcode
+  ///
+  /// With the order of handlers shown above, the ".my-tidy-config" file would
+  /// take precedence over ".clang-tidy" if both reside in the same directory.
+  typedef std::vector<ConfigFileHandler> ConfigFileHandlers;
+
   /// \brief Initializes the \c FileOptionsProvider instance.
   ///
   /// \param GlobalOptions are just stored and returned to the caller of
   /// \c getGlobalOptions.
   ///
   /// \param DefaultOptions are used for all settings not specified in a
-  /// .clang-tidy file.
+  /// configuration file.
   ///
   /// If any of the \param OverrideOptions fields are set, they will override
   /// whatever options are read from the configuration file.
   FileOptionsProvider(const ClangTidyGlobalOptions &GlobalOptions,
                       const ClangTidyOptions &DefaultOptions,
                       const ClangTidyOptions &OverrideOptions);
+
+  /// \brief Initializes the \c FileOptionsProvider instance with a custom set
+  /// of configuration file handlers.
+  ///
+  /// \param GlobalOptions are just stored and returned to the caller of
+  /// \c getGlobalOptions.
+  ///
+  /// \param DefaultOptions are used for all settings not specified in a
+  /// configuration file.
+  ///
+  /// If any of the \param OverrideOptions fields are set, they will override
+  /// whatever options are read from the configuration file.
+  ///
+  /// \param ConfigHandlers specifies a custom set of configuration file
+  /// handlers. Each handler is a pair of configuration file name and a function
+  /// that can parse configuration from this file type. The configuration files
+  /// in each directory are searched for in the order of appearance in
+  /// \p ConfigHandlers.
+  FileOptionsProvider(const ClangTidyGlobalOptions &GlobalOptions,
+                      const ClangTidyOptions &DefaultOptions,
+                      const ClangTidyOptions &OverrideOptions,
+                      const ConfigFileHandlers &ConfigHandlers);
+
   const ClangTidyOptions &getOptions(llvm::StringRef FileName) override;
 
 private:
-  /// \brief Try to read configuration file from \p Directory. If \p Directory
-  /// is empty, use the default value.
-  llvm::ErrorOr<ClangTidyOptions> TryReadConfigFile(llvm::StringRef Directory);
+  /// \brief Try to read configuration files from \p Directory using registered
+  /// \c ConfigHandlers.
+  llvm::Optional<ClangTidyOptions> TryReadConfigFile(llvm::StringRef Directory);
 
   llvm::StringMap<ClangTidyOptions> CachedOptions;
   ClangTidyOptions OverrideOptions;
+  ConfigFileHandlers ConfigHandlers;
 };
 
 /// \brief Parses LineFilter from JSON and stores it to the \p Options.
 std::error_code parseLineFilter(llvm::StringRef LineFilter,
                                 ClangTidyGlobalOptions &Options);
 
 /// \brief Parses configuration from JSON and returns \c ClangTidyOptions or an
 /// error.

clang-tidy/ClangTidyOptions.cpp

 }
 
 FileOptionsProvider::FileOptionsProvider(
     const ClangTidyGlobalOptions &GlobalOptions,
     const ClangTidyOptions &DefaultOptions,
     const ClangTidyOptions &OverrideOptions)
     : DefaultOptionsProvider(GlobalOptions, DefaultOptions),
       OverrideOptions(OverrideOptions) {
+  ConfigHandlers.emplace_back(".clang-tidy", parseConfiguration);
   CachedOptions[""] = DefaultOptions.mergeWith(OverrideOptions);
 }
 
-static const char ConfigFileName[] = ".clang-tidy";
+FileOptionsProvider::FileOptionsProvider(
+    const ClangTidyGlobalOptions &GlobalOptions,
+    const ClangTidyOptions &DefaultOptions,
+    const ClangTidyOptions &OverrideOptions,
+    const FileOptionsProvider::ConfigFileHandlers &ConfigHandlers)
+    : DefaultOptionsProvider(GlobalOptions, DefaultOptions),
+      OverrideOptions(OverrideOptions), ConfigHandlers(ConfigHandlers) {
+  CachedOptions[""] = DefaultOptions.mergeWith(OverrideOptions);
+}
 
 // FIXME: This method has some common logic with clang::format::getStyle().
 // Consider pulling out common bits to a findParentFileWithName function or
 // similar.
 const ClangTidyOptions &FileOptionsProvider::getOptions(StringRef FileName) {
   DEBUG(llvm::dbgs() << "Getting options for file " << FileName << "...\n");
   SmallString<256> FilePath(FileName);
 
   if (std::error_code EC = llvm::sys::fs::make_absolute(FilePath)) {
     llvm::errs() << "Can't make absolute path from " << FileName << ": "
                  << EC.message() << "\n";
     // FIXME: Figure out what to do.
   } else {
     FileName = FilePath;
   }
 
   // Look for a suitable configuration file in all parent directories of the
   // file. Start with the immediate parent directory and move up.
   StringRef Path = llvm::sys::path::parent_path(FileName);
   for (StringRef CurrentPath = Path;;
        CurrentPath = llvm::sys::path::parent_path(CurrentPath)) {
-    llvm::ErrorOr<ClangTidyOptions> Result = std::error_code();
+    llvm::Optional<ClangTidyOptions> Result;
 
     auto Iter = CachedOptions.find(CurrentPath);
     if (Iter != CachedOptions.end())
       Result = Iter->second;
 
     if (!Result)
         Result = TryReadConfigFile(CurrentPath);
 
     if (Result) {
       // Store cached value for all intermediate directories.
       while (Path != CurrentPath) {
         DEBUG(llvm::dbgs() << "Caching configuration for path " << Path
                            << ".\n");
         CachedOptions.GetOrCreateValue(Path, *Result);
         Path = llvm::sys::path::parent_path(Path);
       }
       return CachedOptions.GetOrCreateValue(Path, *Result).getValue();
     }
-    if (Result.getError() != llvm::errc::no_such_file_or_directory) {
-      llvm::errs() << "Error reading " << ConfigFileName << " from " << Path
-                   << ": " << Result.getError().message() << "\n";
-    }
   }
 }
 
-llvm::ErrorOr<ClangTidyOptions>
+llvm::Optional<ClangTidyOptions>
 FileOptionsProvider::TryReadConfigFile(StringRef Directory) {
   assert(!Directory.empty());
 
-  if (!llvm::sys::fs::is_directory(Directory))
-    return make_error_code(llvm::errc::not_a_directory);
+  if (!llvm::sys::fs::is_directory(Directory)) {
+    llvm::errs() << "Error reading configuration from " << Directory
+                 << ": directory doesn't exist.\n";
+    return llvm::None;
+  }
 
+  for (const ConfigFileHandler &ConfigHandler : ConfigHandlers) {
     SmallString<128> ConfigFile(Directory);
-  llvm::sys::path::append(ConfigFile, ".clang-tidy");
+    llvm::sys::path::append(ConfigFile, ConfigHandler.first);
     DEBUG(llvm::dbgs() << "Trying " << ConfigFile << "...\n");
 
     bool IsFile = false;
     // Ignore errors from is_regular_file: we only need to know if we can read
     // the file or not.
     llvm::sys::fs::is_regular_file(Twine(ConfigFile), IsFile);
-
     if (!IsFile)
-    return make_error_code(llvm::errc::no_such_file_or_directory);
+      continue;
 
     llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Text =
         llvm::MemoryBuffer::getFile(ConfigFile.c_str());
-  if (std::error_code EC = Text.getError())
-    return EC;
+    if (std::error_code EC = Text.getError()) {
+      llvm::errs() << "Can't read " << ConfigFile << ": " << EC.message()
+                   << "\n";
+      continue;
+    }
+
     // Skip empty files, e.g. files opened for writing via shell output
     // redirection.
     if ((*Text)->getBuffer().empty())
-    return make_error_code(llvm::errc::no_such_file_or_directory);
+      continue;
     llvm::ErrorOr<ClangTidyOptions> ParsedOptions =
-      parseConfiguration((*Text)->getBuffer());
-  if (ParsedOptions) {
+        ConfigHandler.second((*Text)->getBuffer());
+    if (!ParsedOptions) {
+      llvm::errs() << "Error parsing " << ConfigFile << ": "
+                   << ParsedOptions.getError().message() << "\n";
+      continue;
+    }
+
     ClangTidyOptions Defaults = DefaultOptionsProvider::getOptions(Directory);
     // Only use checks from the config file.
     Defaults.Checks = None;
     return Defaults.mergeWith(*ParsedOptions).mergeWith(OverrideOptions);
   }
-  return ParsedOptions.getError();
+  return llvm::None;
 }
 
 /// \brief Parses -line-filter option and stores it to the \c Options.
 std::error_code parseLineFilter(StringRef LineFilter,
                                 clang::tidy::ClangTidyGlobalOptions &Options) {
   llvm::yaml::Input Input(LineFilter);
   Input >> Options.LineFilter;
   return Input.error();