[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

126

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

183

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

208–217

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

124

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.

154

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

183

Done.

208–217

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

clang-tidy/ClangTidyOptions.h

124

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.

154

Done.

[alexfh]

Ping.

[djasper]

Looks good.

clang-tidy/ClangTidyOptions.cpp

182–192

indentation

clang-tidy/ClangTidyOptions.h

141

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

182–192

Done.

clang-tidy/ClangTidyOptions.h

141

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
+/// will be parsed using \c clang::tidy::parseConfiguration, but a custom set of
+/// configuration file names and parsing functions can be specified.
 class FileOptionsProvider : public DefaultOptionsProvider {
 public:
+  typedef std::pair<std::string, std::function<llvm::ErrorOr<ClangTidyOptions>(
+                                     llvm::StringRef)>> ConfigFileHandler;
+  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.
+  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);
+  llvm::ErrorOr<ClangTidyOptions>
+  TryReadConfigFile(llvm::StringRef Directory,
+                    const ConfigFileHandler &ConfigHandler);
 
   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.push_back(std::make_pair(".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);
 
const ClangTidyOptions &FileOptionsProvider::getOptions(StringRef FileName) {
   for (StringRef CurrentPath = Path;;
        CurrentPath = llvm::sys::path::parent_path(CurrentPath)) {
     llvm::ErrorOr<ClangTidyOptions> Result = std::error_code();
 
     auto Iter = CachedOptions.find(CurrentPath);
     if (Iter != CachedOptions.end())
       Result = Iter->second;
 
-    if (!Result)
-      Result = TryReadConfigFile(CurrentPath);
+    if (!Result) {
+      for (const auto& Handler : ConfigHandlers) {
+        Result = TryReadConfigFile(CurrentPath, Handler);
+        if (Result)
+          break;
+        if (Result.getError() != llvm::errc::no_such_file_or_directory) {
+          llvm::errs() << "Error reading " << Handler.first << " from " << Path
+                       << ": " << Result.getError().message() << "\n";
+        }
+      }
+    }
 
     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>
-FileOptionsProvider::TryReadConfigFile(StringRef Directory) {
+llvm::ErrorOr<ClangTidyOptions> FileOptionsProvider::TryReadConfigFile(
+    StringRef Directory,
+    const FileOptionsProvider::ConfigFileHandler &ConfigHandler) {
   assert(!Directory.empty());
 
   if (!llvm::sys::fs::is_directory(Directory))
     return make_error_code(llvm::errc::not_a_directory);
 
   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);
 
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Text =
       llvm::MemoryBuffer::getFile(ConfigFile.c_str());
   if (std::error_code EC = Text.getError())
     return EC;
   // 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);
   llvm::ErrorOr<ClangTidyOptions> ParsedOptions =
-      parseConfiguration((*Text)->getBuffer());
+      ConfigHandler.second((*Text)->getBuffer());
   if (ParsedOptions) {
     ClangTidyOptions Defaults = DefaultOptionsProvider::getOptions(Directory);
     // Only use checks from the config file.
     Defaults.Checks = None;
     return Defaults.mergeWith(*ParsedOptions).mergeWith(OverrideOptions);
   }
   return ParsedOptions.getError();
 }