[clang-tidy] Add support for boolean check options.

[klimek]

clang-tidy/ClangTidy.h

45–46

If there's a reason not to use StringRef instead of const char*, it should be in ALL CAPS, RED AND BOLD in the comment above ;)

62–74

Why do we want unknown values to give Default? If we do, we should put it into the comments; can we rather make them given an error while parsing, and then do:
return get(LocalName, Default ? "true" : "false") == "true";

69–74

Will is_integral trigger for 128 bit integral values, and then narrow?

[curdeius]

We can generalize go a step further get() and store() to all types that specialize llvm::yaml traits.
This could remove the code duplication and simplify the creation of clang-tidy checks having options of enumeration type.

clang-tidy/ClangTidy.h

45–46

What's the reason for changing std::string Default to const char* Default?
It "breaks" somehow the symmetry of T get(StringRef, T), i.e. taking T type as Default and returning T.

67

That's essentially the same code as in llvm/lib/Support/YAMLTraits.cpp::651.

67–74

Couldn't we just generalize this approach to all the types that specialize llvm::yaml::XYZTraits (XYZ being one of Scalar/ScalarEnumeration/...) ?

An ugly hack (working at least for enums specializing ScalarEnumerationTraits) would be:

template <typename T>
typename std::enable_if<!std::is_integral<T>::value &&
                            !std::is_convertible<T, std::string>::value,
                        T>::type
get(StringRef LocalName, T Default) const {
  std::string Value = get(LocalName, "");
  T Result = Default;
  if (!Value.empty()) {
    std::stringstream Content;
    Content << LocalName.str() << ": " << Value;
    llvm::yaml::Input Input(Content.str());
    Input.setCurrentDocument();
    Input.mapOptional(LocalName.data(), Result);
    if (Input.error())
      Result = Default;
  }
  return Result;
}

The hack is ugly since it uses Input::setCurrentDocument() (which should probably be private) and Input::mapOptional() and not Input::operator>>. That is so, because otherwise we would need creating a type specializing llvm::yaml::MappingTraits which would essentially do an operation equivalent to Input.mapOptional(LocalName.str(), Result);.

78–79

Similarly to above get() method, one can generalize the approach.
Another ugly hack would be:

template <typename T>
typename std::enable_if<!std::is_integral<T>::value &&
                            !std::is_convertible<T, std::string>::value,
                        void>::type
store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,
      T Value) const {
  std::string Text;
  llvm::raw_string_ostream Content(Text);
  llvm::yaml::Output Output(Content);
  Output.beginMapping();
  if (Output.preflightDocument(0))
    Output.mapOptional(LocalName.data(), Value);
  Output.endMapping();

  auto T = Content.str();
  // Skip past ": " between key and value.
  auto Off = T.find(':', LocalName.size());
  assert(Off != std::string::npos);
  T = T.substr(Off + 2, std::string::npos);
  // FIXME(mkurdej): Strip enclosing apostrophes "'" if present.
  Options[NamePrefix + LocalName.str()] = T;
}

[alexfh]

clang-tidy/ClangTidy.h

67–74

Couldn't we just generalize this approach to all the types that specialize llvm::yaml::XYZTraits (XYZ being one of Scalar/ScalarEnumeration/...) ?

That sounds interesting. I'll try something along these lines.

[alexfh]

Use llvm::yaml::ScalarTraits to convert all built-it and string types. No
support for enumerations though, as it is bound tighter to llvm::yaml::IO, and
no error handling (not sure if we need it).

[alexfh]

clang-tidy/ClangTidy.h

62–74

We can't error out early enough as check options are parsed after the configuration is read. So I don't currently see a better way than to fall back to default.

[klimek]

clang-tidy/ClangTidy.h

62–74

Fall back to false?

[alexfh]

clang-tidy/ClangTidy.h

62–74

Could you explain why this would be better?

[klimek]

clang-tidy/ClangTidy.h

62–74

If we catch the error in the config later on, and give a nice error, it doesn't matter, and the code gets simpler.

If we cannot catch the error, I would like an explicitly given config to not lead to different behavior depending on the default value; I find that unexpected. Plus, I think it would make the code simpler.

[alexfh]

clang-tidy/ClangTidy.h

62–74

If we catch the error in the config later on, and give a nice error,

Not sure yet if this can be done nicely.

it doesn't matter, and the code gets simpler.

It doesn't make the code simpler, as we still need to handle missing value and return Default.

If we cannot catch the error, I would like an explicitly given config to not lead to different behavior depending on the default value; I find that unexpected.

Why? On the opposite, I think that the case of invalid value is close to the case of missing value, so using the same default is more intuitive.

Plus, I think it would make the code simpler.

Not really. The code is now common for all types having ScalarTraits<>. It also falls back to Default in case of error, and there I don't see another reasonable value to fall back to in case the parsing fails.

[klimek]

clang-tidy/ClangTidy.h

62–74

Ok, I'm convinced:
If this is really the same handling for all scalar values, I find it significantly worse if we silently eat up parsing errors. I'd prefer crashing with an error message if that's the only way possible.

[alexfh]

Let the check decide what to do with invalid option values.

[alexfh]

clang-tidy/ClangTidy.h

62–74

Crashing doesn't seem like a the best solution. We could (mis)use diagnostic output to report configuration errors. WDYT?

For now, I've changed OptionsView::get<> to propagate errors to the caller. And the caller can decide whether to crash in case of an error or handle it in a different way. I've changed two current users of this method to use ErrorOr<>::operator* which asserts that there's no value. Do we want to add some kind of a getValueOrCrash method to ErrorOr<> which would use report_fatal_error instead?

[curdeius]

clang-tidy/ClangTidy.h

45–54

Won't it be counterintuitive for the user of this code that get(..., string) returns a string and that get(..., T) returns ErrorOr<T>?

[alexfh]

clang-tidy/ClangTidy.h

45–54

The difference is that the string version doesn't do any parsing, so it can't encounter any errors. So maybe it should be named differently from the template version to avoid confusion.

[curdeius]

clang-tidy/ClangTidy.h

45–54

Yes, I know that, but still it's a bit weird in my eyes to have functions with same name behaving as differently as this.
Renaming it is one valid approach.
Another one would be to add an argument indicating that something went wrong, e.g. get(..., T Default, bool *Invalid = nullptr).
It would be up to the user to handle the error or not (on error, get will return Default).
In any case, we don't pass any more information with error_code than with a boolean...

What are your thoughts about it?

[alexfh]

clang-tidy/ClangTidy.h

45–54

ScalarTraits::input returns a StringRef describing the error. We can also use StringRef to avoid impedance mismatch and make use of these strings in our error messages.

Or maybe ScalarTraits<> should be changed to use std::error_code with custom errors. I'll ask the authors' opinion.

[klimek]

clang-tidy/ClangTidy.h

45–54

I'd vote for renaming.

62–74

+1 to some form of diagnostic. Also +1 for handing to to the caller.
Is there a way where the caller can just ask whether there's a value?

[alexfh]

clang-tidy/ClangTidy.h

62–74

We can add OptionsView::get overloads that return Optional<Something> (though not sure why would callers bother using it instead of the versions with default).

[klimek]

clang-tidy/ClangTidy.h

62–74

I meant as opposed to an error. Otherwise I don't understand your last comment above:
""" I've changed two current users of this method to use ErrorOr<>::operator* which asserts that there's no value. Do we want to add some kind of a getValueOrCrash method to ErrorOr<> which would use report_fatal_error instead?"""

[alexfh]

Complete rewrite of the options access and validation code. Now modules can
provide typed default values for the options defined by module's checks.
Validation happens during option parsing and results in the same kind of error
reporting as other configuration format errors.

[alexfh]

Actually, the actual command-line workflow is broken now, so I'm working on fixing it and adding a test. But I believe, the fix won't require much change, so it shouldn't postpone the review.

[alexfh]

Fix for the command-line workflow (proper tests for this will be later).

[alexfh]

Ping.

[alexfh]

Rebased the patch on top of D5821.

PTAL.

[klimek]

clang-tidy/ClangTidyOptions.h

79–97

So, if we want to roll with having the scalar transformations on the class, I'd just make Value private and overload get() and store() for string types - that would also make the outer code symmetrical.

Second, I'm slightly confused about storing the Validate function with the option - it seems to be completely unrelated (it's about the option type, not about the option value).

clang-tidy/ClangTidy.h

 #include "ClangTidyDiagnosticConsumer.h"
 #include "ClangTidyOptions.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Refactoring.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/Support/YAMLTraits.h"
 #include <memory>
 #include <type_traits>
 #include <vector>
 
 namespace clang {
 
 class CompilerInstance;
 namespace tooling {
 class CompilationDatabase;
 }
 
 namespace tidy {
 
 /// \brief Provides access to the \c ClangTidyCheck options via check-local
 /// names.
 ///
 /// Methods of this class prepend <tt>CheckName + "."</tt> to translate
 /// check-local option names to global option names.
 class OptionsView {
 public:
   /// \brief Initializes the instance using \p CheckName + "." as a prefix.
   OptionsView(StringRef CheckName,
               const ClangTidyOptions::OptionMap &CheckOptions);
 
-  /// \brief Read a named option from the \c Context.
-  ///
-  /// Reads the option with the check-local name \p LocalName from the
-  /// \c CheckOptions. If the corresponding key is not present, returns
-  /// \p Default.
-  std::string get(StringRef LocalName, std::string Default) const;
-
-  /// \brief Read a named option from the \c Context and parse it as an integral
-  /// type \c T.
-  ///
-  /// Reads the option with the check-local name \p LocalName from the
-  /// \c CheckOptions. If the corresponding key is not present, returns
-  /// \p Default.
+  std::string get(StringRef LocalName, StringRef Default) const {
+    const auto &Iter = CheckOptions.find(NamePrefix + LocalName.str());
+    if (Iter != CheckOptions.end())
+      return Iter->second.Value;
+    return Default;
+  }
+
   template <typename T>
-  typename std::enable_if<std::is_integral<T>::value, T>::type
-  get(StringRef LocalName, T Default) const {
-    std::string Value = get(LocalName, "");
-    T Result = Default;
-    if (!Value.empty())
-      StringRef(Value).getAsInteger(10, Result);
-    return Result;
+  typename std::enable_if<llvm::yaml::has_ScalarTraits<T>::value, T>::type
+  get(StringRef LocalName, const T &Default) const {
+    const auto &Iter = CheckOptions.find(NamePrefix + LocalName.str());
+    if (Iter != CheckOptions.end())
+      return Iter->second.get(Default);
+    return Default;
   }
 
-  /// \brief Stores an option with the check-local name \p LocalName with string
-  /// value \p Value to \p Options.
-  void store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,
-             StringRef Value) const;
-
-  /// \brief Stores an option with the check-local name \p LocalName with
-  /// \c int64_t value \p Value to \p Options.
-  void store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,
-             int64_t Value) const;
+  ClangTidyOptions::Option &get(ClangTidyOptions::OptionMap &Options,
+                                StringRef LocalName) const {
+    return Options[NamePrefix + LocalName.str()];
+  }
 
 private:
   std::string NamePrefix;
   const ClangTidyOptions::OptionMap &CheckOptions;
 };
 
 /// \brief Base class for all clang-tidy checks.
 ///
 /// To implement a \c ClangTidyCheck, write a subclass and overwrite some of the
 /// base class's methods. E.g. to implement a check that validates namespace
 /// declarations, overwrite \c registerMatchers:
 ///
 /// \code
 /// registerMatchers(ast_matchers::MatchFinder *Finder) {
 ///   Finder->addMatcher(namespaceDecl().bind("namespace"), this);
 /// }
 /// \endcode
 ///
 /// and then overwrite \c check(const MatchResult &Result) to do the actual
 /// check for each match.
 ///
 /// A new \c ClangTidyCheck instance is created per translation unit.

clang-tidy/ClangTidy.cpp

void ClangTidyCheck::run(const ast_matchers::MatchFinder::MatchResult &Result) {
   Context->setSourceManager(Result.SourceManager);
   check(Result);
 }
 
 OptionsView::OptionsView(StringRef CheckName,
                          const ClangTidyOptions::OptionMap &CheckOptions)
     : NamePrefix(CheckName.str() + "."), CheckOptions(CheckOptions) {}
 
-std::string OptionsView::get(StringRef LocalName, std::string Default) const {
-  const auto &Iter = CheckOptions.find(NamePrefix + LocalName.str());
-  if (Iter != CheckOptions.end())
-    return Iter->second;
-  return Default;
-}
-
-void OptionsView::store(ClangTidyOptions::OptionMap &Options,
-                        StringRef LocalName, StringRef Value) const {
-  Options[NamePrefix + LocalName.str()] = Value;
-}
-
-void OptionsView::store(ClangTidyOptions::OptionMap &Options,
-                        StringRef LocalName, int64_t Value) const {
-  store(Options, LocalName, llvm::itostr(Value));
-}
-
 std::vector<std::string> getCheckNames(const ClangTidyOptions &Options) {
   clang::tidy::ClangTidyContext Context(
       llvm::make_unique<DefaultOptionsProvider>(ClangTidyGlobalOptions(),
                                                 Options));
   ClangTidyASTConsumerFactory Factory(Context);
   return Factory.getCheckNames();
 }
 

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 "llvm/Support/YAMLTraits.h"
 #include <functional>
 #include <map>
 #include <string>
 #include <system_error>
 #include <utility>
 #include <vector>
 
 namespace clang {
struct ClangTidyOptions {
   llvm::Optional<bool> AnalyzeTemporaryDtors;
 
   /// \brief Specifies the name or e-mail of the user running clang-tidy.
   ///
   /// This option is used, for example, to place the correct user name in TODO()
   /// 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;
+  struct Option {
+    template <typename T>
+    typename std::enable_if<llvm::yaml::has_ScalarTraits<T>::value, T>::type
+    get(const T &Default) const {
+      T Result = Default;
+      llvm::yaml::ScalarTraits<T>::input(Value, nullptr, Result);
+      return Result;
+    }
+
+    template <typename T>
+    typename std::enable_if<llvm::yaml::has_ScalarTraits<T>::value, void>::type
+    store(const T &TypedValue) {
+      llvm::raw_string_ostream OS(Value);
+      llvm::yaml::ScalarTraits<T>::output(TypedValue, nullptr, OS);
+    }
+
+    std::string Value;
+    std::function<llvm::StringRef(llvm::StringRef)> Validate;
+  };
+
+  template <typename T>
+  static typename std::enable_if<llvm::yaml::has_ScalarTraits<T>::value,
+                                 Option>::type
+  TypedOption(const T &Default) {
+    Option Result;
+    Result.store(Default);
+    Result.Validate = [](llvm::StringRef Input) {
+      T Dummy;
+      return llvm::yaml::ScalarTraits<T>::input(Input, nullptr, Dummy);
+    };
+    return Result;
+  }
+
+  typedef std::map<std::string, Option> OptionMap;
+  typedef std::pair<std::string, Option> OptionItem;
 
   /// \brief Key-value mapping used to store check-specific options.
   OptionMap CheckOptions;
 };
 
 /// \brief Abstract interface for retrieving various ClangTidy options.
 class ClangTidyOptionsProvider {
 public:

 /// 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;
+  typedef std::pair<std::string,
+                    std::function<llvm::ErrorOr<ClangTidyOptions>(
+                        llvm::StringRef, const ClangTidyOptions &)>>
+      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:

 };
 
 /// \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.
-llvm::ErrorOr<ClangTidyOptions> parseConfiguration(llvm::StringRef Config);
+llvm::ErrorOr<ClangTidyOptions>
+parseConfiguration(llvm::StringRef Config, const ClangTidyOptions &Defaults);
 
 /// \brief Serializes configuration to a YAML-encoded string.
 std::string configurationAsText(const ClangTidyOptions &Options);
 
 } // end namespace tidy
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANG_TIDY_OPTIONS_H

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(ClangTidyOptions::OptionItem)
 
 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;
static StringRef validate(IO &io, FileFilter &File) {
     for (const FileFilter::LineRange &Range : File.LineRanges) {
       if (Range.first <= 0 || Range.second <= 0)
         return "Invalid line range";
     }
     return StringRef();
   }
 };
 
-template <> struct MappingTraits<ClangTidyOptions::StringPair> {
-  static void mapping(IO &IO, ClangTidyOptions::StringPair &KeyValue) {
+template <> struct MappingTraits<ClangTidyOptions::OptionItem> {
+  static void mapping(IO &IO, ClangTidyOptions::OptionItem &KeyValue) {
     IO.mapRequired("key", KeyValue.first);
-    IO.mapRequired("value", KeyValue.second);
+    IO.mapRequired("value", KeyValue.second.Value);
+    if (auto Defaults =
+            reinterpret_cast<const ClangTidyOptions *>(IO.getContext())) {
+      auto Iter = Defaults->CheckOptions.find(KeyValue.first);
+      if (Iter != Defaults->CheckOptions.end() && Iter->second.Validate) {
+        StringRef Error = Iter->second.Validate(KeyValue.second.Value);
+        if (!Error.empty())
+          IO.setError(Error);
+      }
+    }
   }
 };
 
 struct NOptionMap {
   NOptionMap(IO &) {}
   NOptionMap(IO &, const ClangTidyOptions::OptionMap &OptionMap)
       : Options(OptionMap.begin(), OptionMap.end()) {}
   ClangTidyOptions::OptionMap denormalize(IO &) {
     ClangTidyOptions::OptionMap Map;
     for (const auto &KeyValue : Options)
       Map[KeyValue.first] = KeyValue.second;
     return Map;
   }
-  std::vector<ClangTidyOptions::StringPair> Options;
+  std::vector<ClangTidyOptions::OptionItem> Options;
 };
 
 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);
FileOptionsProvider::TryReadConfigFile(StringRef Directory) {
   assert(!Directory.empty());
 
   if (!llvm::sys::fs::is_directory(Directory)) {
     llvm::errs() << "Error reading configuration from " << Directory
                  << ": directory doesn't exist.\n";
     return llvm::None;
   }
 
+  ClangTidyOptions Defaults = DefaultOptionsProvider::getOptions(Directory);
+  // Only use checks from the config file.
+  Defaults.Checks = None;
+
   for (const ConfigFileHandler &ConfigHandler : ConfigHandlers) {
     SmallString<128> ConfigFile(Directory);
     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.
if (std::error_code EC = Text.getError()) {
       continue;
     }
 
     // Skip empty files, e.g. files opened for writing via shell output
     // redirection.
     if ((*Text)->getBuffer().empty())
       continue;
     llvm::ErrorOr<ClangTidyOptions> 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);
+        ConfigHandler.second((*Text)->getBuffer(), Defaults);
+    if (ParsedOptions)
+      return ParsedOptions->mergeWith(OverrideOptions);
+
   }
   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();
 }
 
-llvm::ErrorOr<ClangTidyOptions> parseConfiguration(StringRef Config) {
+llvm::ErrorOr<ClangTidyOptions>
+parseConfiguration(StringRef Config, const ClangTidyOptions &Defaults) {
   llvm::yaml::Input Input(Config);
-  ClangTidyOptions Options;
+  Input.setContext(const_cast<ClangTidyOptions *>(&Defaults));
+  ClangTidyOptions Options = Defaults;
   Input >> Options;
   if (Input.error())
     return Input.error();
   return Options;
 }
 
 std::string configurationAsText(const ClangTidyOptions &Options) {
   std::string Text;

clang-tidy/google/GoogleTidyModule.cpp

void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<readability::BracesAroundStatementsCheck>(
         "google-readability-braces-around-statements");
   }
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     auto &Opts = Options.CheckOptions;
     Opts["google-readability-braces-around-statements.ShortStatementLines"] =
-        "1";
-    Opts["google-readability-namespace-comments.ShortNamespaceLines"] = "1";
-    Opts["google-readability-namespace-comments.SpacesBeforeComments"] = "2";
+        ClangTidyOptions::TypedOption(1U);
+    Opts["google-readability-namespace-comments.ShortNamespaceLines"] =
+        ClangTidyOptions::TypedOption(1U);
+    Opts["google-readability-namespace-comments.SpacesBeforeComments"] =
+        ClangTidyOptions::TypedOption(2U);
     return Options;
   }
 };
 
 // Register the GoogleTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<GoogleModule> X("google-module",
                                                     "Adds Google lint checks.");
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the GoogleModule.
 volatile int GoogleModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tidy/readability/BracesAroundStatementsCheck.cpp

 BracesAroundStatementsCheck::BracesAroundStatementsCheck(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       // Always add braces by default.
       ShortStatementLines(Options.get("ShortStatementLines", 0U)) {}
 
 void
 BracesAroundStatementsCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
-  Options.store(Opts, "ShortStatementLines", ShortStatementLines);
+  Options.get(Opts, "ShortStatementLines").store(ShortStatementLines);
 }
 
 void BracesAroundStatementsCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(ifStmt().bind("if"), this);
   Finder->addMatcher(whileStmt().bind("while"), this);
   Finder->addMatcher(doStmt().bind("do"), this);
   Finder->addMatcher(forStmt().bind("for"), this);
   Finder->addMatcher(forRangeStmt().bind("for-range"), this);

clang-tidy/readability/FunctionSize.cpp

 
 FunctionSizeCheck::FunctionSizeCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       LineThreshold(Options.get("LineThreshold", -1U)),
       StatementThreshold(Options.get("StatementThreshold", 800U)),
       BranchThreshold(Options.get("BranchThreshold", -1U)) {}
 
 void FunctionSizeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
-  Options.store(Opts, "LineThreshold", LineThreshold);
-  Options.store(Opts, "StatementThreshold", StatementThreshold);
-  Options.store(Opts, "BranchThreshold", BranchThreshold);
+  Options.get(Opts, "LineThreshold").store(LineThreshold);
+  Options.get(Opts, "StatementThreshold").store(StatementThreshold);
+  Options.get(Opts, "BranchThreshold").store(BranchThreshold);
 }
 
 void FunctionSizeCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       functionDecl(
           unless(isInstantiated()),
           forEachDescendant(
               stmt(unless(compoundStmt()),

clang-tidy/readability/NamespaceCommentCheck.cpp

NamespaceCommentCheck::NamespaceCommentCheck(StringRef Name,
     : ClangTidyCheck(Name, Context),
       NamespaceCommentPattern("^/[/*] *(end (of )?)? *(anonymous|unnamed)? *"
                               "namespace( +([a-zA-Z0-9_]+))?\\.? *(\\*/)?$",
                               llvm::Regex::IgnoreCase),
       ShortNamespaceLines(Options.get("ShortNamespaceLines", 1u)),
       SpacesBeforeComments(Options.get("SpacesBeforeComments", 1u)) {}
 
 void NamespaceCommentCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
-  Options.store(Opts, "ShortNamespaceLines", ShortNamespaceLines);
-  Options.store(Opts, "SpacesBeforeComments", SpacesBeforeComments);
+  Options.get(Opts, "ShortNamespaceLines").store(ShortNamespaceLines);
+  Options.get(Opts, "SpacesBeforeComments").store(SpacesBeforeComments);
 }
 
 void NamespaceCommentCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(namespaceDecl().bind("namespace"), this);
 }
 
 bool locationsInSameFile(const SourceManager &Sources, SourceLocation Loc1,
                          SourceLocation Loc2) {

clang-tidy/tool/ClangTidyMain.cpp

 std::unique_ptr<ClangTidyOptionsProvider> createOptionsProvider() {
   ClangTidyGlobalOptions GlobalOptions;
   if (std::error_code Err = parseLineFilter(LineFilter, GlobalOptions)) {
     llvm::errs() << "Invalid LineFilter: " << Err.message() << "\n\nUsage:\n";
     llvm::cl::PrintHelpMessage(/*Hidden=*/false, /*Categorized=*/true);
     return nullptr;
   }
 
-  ClangTidyOptions DefaultOptions;
+  ClangTidyOptions DefaultOptions = ClangTidyOptions::getDefaults();
   DefaultOptions.Checks = DefaultChecks;
   DefaultOptions.HeaderFilterRegex = HeaderFilter;
   DefaultOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors;
   DefaultOptions.User = llvm::sys::Process::GetEnv("USER");
   // USERNAME is used on Windows.
   if (!DefaultOptions.User)
     DefaultOptions.User = llvm::sys::Process::GetEnv("USERNAME");
 
   ClangTidyOptions OverrideOptions;
   if (Checks.getNumOccurrences() > 0)
     OverrideOptions.Checks = Checks;
   if (HeaderFilter.getNumOccurrences() > 0)
     OverrideOptions.HeaderFilterRegex = HeaderFilter;
   if (AnalyzeTemporaryDtors.getNumOccurrences() > 0)
     OverrideOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors;
 
   if (!Config.empty()) {
     if (llvm::ErrorOr<ClangTidyOptions> ParsedConfig =
-            parseConfiguration(Config)) {
+            parseConfiguration(Config, DefaultOptions)) {
       return llvm::make_unique<DefaultOptionsProvider>(
-          GlobalOptions, ClangTidyOptions::getDefaults()
-                             .mergeWith(DefaultOptions)
-                             .mergeWith(*ParsedConfig)
-                             .mergeWith(OverrideOptions));
+          GlobalOptions, ParsedConfig->mergeWith(OverrideOptions));
     } else {
       llvm::errs() << "Error: invalid configuration specified.\n"
                    << ParsedConfig.getError().message() << "\n";
       return nullptr;
     }
   }
   return llvm::make_unique<FileOptionsProvider>(GlobalOptions, DefaultOptions,
                                                 OverrideOptions);

unittests/clang-tidy/ClangTidyOptionsTest.cpp

+#include "ClangTidy.h"
 #include "ClangTidyOptions.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tidy {
 namespace test {
 
 TEST(ParseLineFilter, EmptyFilter) {
TEST(ParseLineFilter, ValidFilter) {
   EXPECT_EQ(1000u, Options.LineFilter[2].LineRanges[0].second);
 }
 
 TEST(ParseConfiguration, ValidConfiguration) {
   llvm::ErrorOr<ClangTidyOptions> Options =
       parseConfiguration("Checks: \"-*,misc-*\"\n"
                          "HeaderFilterRegex: \".*\"\n"
                          "AnalyzeTemporaryDtors: true\n"
-                         "User: some.user");
+                         "User: some.user",
+                         ClangTidyOptions());
   EXPECT_TRUE(!!Options);
   EXPECT_EQ("-*,misc-*", *Options->Checks);
   EXPECT_EQ(".*", *Options->HeaderFilterRegex);
   EXPECT_TRUE(*Options->AnalyzeTemporaryDtors);
   EXPECT_EQ("some.user", *Options->User);
 }
 
+TEST(ParseConfiguration, CheckOptions) {
+  llvm::ErrorOr<ClangTidyOptions> Options =
+      parseConfiguration("CheckOptions:\n"
+                         "  - key: test-check.String\n"
+                         "    value: ' qwe !@#* '\n"
+                         "  - key: test-check.IntegerPositive\n"
+                         "    value: 12345\n"
+                         "  - key: test-check.IntegerNegative\n"
+                         "    value: -67890\n"
+                         "  - key: test-check.IntegerInvalid\n"
+                         "    value: asdf\n"
+                         "  - key: test-check.BooleanTrue\n"
+                         "    value: true\n"
+                         "  - key: test-check.BooleanFalse\n"
+                         "    value: false\n"
+                         "  - key: test-check.BooleanInvalid\n"
+                         "    value: asdf\n",
+                         ClangTidyOptions());
+  EXPECT_TRUE(!!Options);
+  OptionsView Opt("test-check", Options->CheckOptions);
+  EXPECT_EQ("qqq", Opt.get("NonExistent", "qqq"));
+  EXPECT_EQ(" qwe !@#* ", Opt.get("String", ""));
+
+  EXPECT_EQ(-123, Opt.get("NonExistent", -123));
+  EXPECT_EQ(123, Opt.get("IntegerInvalid", 123));
+  EXPECT_EQ(12345, Opt.get("IntegerPositive", 0));
+  EXPECT_EQ(-67890, Opt.get("IntegerNegative", 0));
+
+  EXPECT_EQ(123U, Opt.get("NonExistent", 123U));
+  EXPECT_EQ(123U, Opt.get("IntegerNegative", 123U));
+  EXPECT_EQ(12345U, Opt.get("IntegerPositive", 0U));
+
+  EXPECT_TRUE(Opt.get("NonExistent", true));
+  EXPECT_FALSE(Opt.get("NonExistent", false));
+  EXPECT_TRUE(Opt.get("BooleanTrue", false));
+  EXPECT_FALSE(Opt.get("BooleanFalse", true));
+  EXPECT_TRUE(Opt.get("BooleanInvalid", true));
+}
+
+TEST(ParseConfiguration, CheckOptionsValidation) {
+  ClangTidyOptions Defaults;
+  Defaults.CheckOptions["test-check.BooleanInvalid"] =
+      ClangTidyOptions::TypedOption(true);
+  llvm::ErrorOr<ClangTidyOptions> Options =
+      parseConfiguration("CheckOptions:\n"
+                         "  - key: test-check.BooleanInvalid\n"
+                         "    value: asdf\n",
+                         Defaults);
+  EXPECT_FALSE(!!Options);
+}
+
 } // namespace test
 } // namespace tidy
 } // namespace clang