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.
http://reviews.llvm.org/D5602
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/ClangTidyOptions.cpp
clang-tidy/ClangTidyOptions.h
clang-tidy/google/GoogleTidyModule.cpp
clang-tidy/readability/BracesAroundStatementsCheck.cpp
clang-tidy/readability/FunctionSize.cpp
clang-tidy/readability/NamespaceCommentCheck.cpp
clang-tidy/tool/ClangTidyMain.cpp
unittests/clang-tidy/ClangTidyOptionsTest.cpp
Index: clang-tidy/ClangTidy.cpp
===================================================================
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -319,23 +319,6 @@
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(),
Index: clang-tidy/ClangTidy.h
===================================================================
--- clang-tidy/ClangTidy.h
+++ clang-tidy/ClangTidy.h
@@ -18,6 +18,7 @@
#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>
@@ -42,38 +43,26 @@
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;
Index: clang-tidy/ClangTidyOptions.cpp
===================================================================
--- clang-tidy/ClangTidyOptions.cpp
+++ clang-tidy/ClangTidyOptions.cpp
@@ -26,7 +26,7 @@
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 {
@@ -59,10 +59,19 @@
}
};
-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);
+ }
+ }
}
};
@@ -76,7 +85,7 @@
Map[KeyValue.first] = KeyValue.second;
return Map;
}
- std::vector<ClangTidyOptions::StringPair> Options;
+ std::vector<ClangTidyOptions::OptionItem> Options;
};
template <> struct MappingTraits<ClangTidyOptions> {
@@ -217,14 +226,13 @@
// redirection.
if ((*Text)->getBuffer().empty())
return make_error_code(llvm::errc::no_such_file_or_directory);
+ ClangTidyOptions Defaults = DefaultOptionsProvider::getOptions(Directory);
+ // Only use checks from the config file.
+ Defaults.Checks = None;
llvm::ErrorOr<ClangTidyOptions> ParsedOptions =
- parseConfiguration((*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);
- }
+ parseConfiguration((*Text)->getBuffer(), Defaults);
+ if (ParsedOptions)
+ return ParsedOptions->mergeWith(OverrideOptions);
return ParsedOptions.getError();
}
@@ -236,9 +244,11 @@
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();
Index: clang-tidy/ClangTidyOptions.h
===================================================================
--- clang-tidy/ClangTidyOptions.h
+++ clang-tidy/ClangTidyOptions.h
@@ -14,6 +14,8 @@
#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>
@@ -74,8 +76,41 @@
/// 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;
@@ -148,7 +183,8 @@
/// \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);
Index: clang-tidy/google/GoogleTidyModule.cpp
===================================================================
--- clang-tidy/google/GoogleTidyModule.cpp
+++ clang-tidy/google/GoogleTidyModule.cpp
@@ -64,9 +64,11 @@
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;
}
};
Index: clang-tidy/readability/BracesAroundStatementsCheck.cpp
===================================================================
--- clang-tidy/readability/BracesAroundStatementsCheck.cpp
+++ clang-tidy/readability/BracesAroundStatementsCheck.cpp
@@ -119,7 +119,7 @@
void
BracesAroundStatementsCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
- Options.store(Opts, "ShortStatementLines", ShortStatementLines);
+ Options.get(Opts, "ShortStatementLines").store(ShortStatementLines);
}
void BracesAroundStatementsCheck::registerMatchers(MatchFinder *Finder) {
Index: clang-tidy/readability/FunctionSize.cpp
===================================================================
--- clang-tidy/readability/FunctionSize.cpp
+++ clang-tidy/readability/FunctionSize.cpp
@@ -23,9 +23,9 @@
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) {
Index: clang-tidy/readability/NamespaceCommentCheck.cpp
===================================================================
--- clang-tidy/readability/NamespaceCommentCheck.cpp
+++ clang-tidy/readability/NamespaceCommentCheck.cpp
@@ -29,8 +29,8 @@
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) {
Index: clang-tidy/tool/ClangTidyMain.cpp
===================================================================
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -169,13 +169,11 @@
OverrideOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors;
if (!Config.empty()) {
- if (llvm::ErrorOr<ClangTidyOptions> ParsedConfig =
- parseConfiguration(Config)) {
+ if (llvm::ErrorOr<ClangTidyOptions> ParsedConfig = parseConfiguration(
+ Config,
+ ClangTidyOptions::getDefaults().mergeWith(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";
Index: unittests/clang-tidy/ClangTidyOptionsTest.cpp
===================================================================
--- unittests/clang-tidy/ClangTidyOptionsTest.cpp
+++ unittests/clang-tidy/ClangTidyOptionsTest.cpp
@@ -1,3 +1,4 @@
+#include "ClangTidy.h"
#include "ClangTidyOptions.h"
#include "gtest/gtest.h"
@@ -59,14 +60,66 @@
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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
