[clang-tidy] Alternative approach to OptionsView::get

[alexfh]

Summary

This patch is not for review, it's only a demonstration of a not very elegant approach to changing OptionsView::get.

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/Errc.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 {
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;
+  /// \c llvm::None.
+  llvm::Optional<std::string> get(StringRef LocalName) const;
 
-  /// \brief Read a named option from the \c Context and parse it as an integral
-  /// type \c T.
+  /// \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 the type
+  /// \c T.
+  ///
+  /// Reads the option with the check-local name \p LocalName from the
+  /// \c CheckOptions, parses the value as the type \c T using
+  /// \c llvm::yaml::ScalarTraits<T> and stores it to \p Value. On success
+  /// returns an empty \c StringRef. If the corresponding key is not present,
+  /// doesn't change the \p Value and returns an empty \c StringRef. If a
+  /// parsing error occurs, returns a non-empty \c StringRef describing the
+  /// error.
   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,
+                          llvm::StringRef>::type
+  parseOption(StringRef LocalName, T& Value) const {
+    if (llvm::Optional<std::string> String = get(LocalName))
+      return llvm::yaml::ScalarTraits<T>::input(*String, nullptr, Value);
+    return llvm::StringRef();
   }
 
   /// \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

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) {}
 
+llvm::Optional<std::string> OptionsView::get(StringRef LocalName) const {
+  const auto &Iter = CheckOptions.find(NamePrefix + LocalName.str());
+  if (Iter != CheckOptions.end())
+    return Iter->second;
+  return llvm::None;
+}
+
 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,

clang-tidy/readability/BracesAroundStatementsCheck.h

private:
   void checkStmt(const ast_matchers::MatchFinder::MatchResult &Result,
                  const Stmt *S, SourceLocation StartLoc,
                  SourceLocation EndLocHint = SourceLocation());
   template <typename IfOrWhileStmt>
   SourceLocation findRParenLoc(const IfOrWhileStmt *S, const SourceManager &SM,
                                const ASTContext *Context);
 
 private:
-  const unsigned ShortStatementLines;
+  unsigned ShortStatementLines;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_BRACESAROUNDSTATEMENTSCHECK_H

clang-tidy/readability/BracesAroundStatementsCheck.cpp

 }
 
 } // namespace
 
 BracesAroundStatementsCheck::BracesAroundStatementsCheck(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       // Always add braces by default.
-      ShortStatementLines(Options.get("ShortStatementLines", 0U)) {}
+      ShortStatementLines(0U) {
+  StringRef Error =
+      Options.parseOption("ShortStatementLines", ShortStatementLines);
+  if (!Error.empty())
+    llvm::errs() << "Invalid value for ShortStatementLines: " << Error << "\n";
+}
 
 void
 BracesAroundStatementsCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "ShortStatementLines", ShortStatementLines);
 }
 
 void BracesAroundStatementsCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(ifStmt().bind("if"), this);

clang-tidy/readability/FunctionSize.h

 private:
   struct FunctionInfo {
     FunctionInfo() : Lines(0), Statements(0), Branches(0) {}
     unsigned Lines;
     unsigned Statements;
     unsigned Branches;
   };
 
-  const unsigned LineThreshold;
-  const unsigned StatementThreshold;
-  const unsigned BranchThreshold;
+  unsigned LineThreshold;
+  unsigned StatementThreshold;
+  unsigned BranchThreshold;
 
   llvm::DenseMap<const FunctionDecl *, FunctionInfo> FunctionInfos;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_FUNCTIONSIZE_H

clang-tidy/readability/FunctionSize.cpp

 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 FunctionSizeCheck::FunctionSizeCheck(StringRef Name, ClangTidyContext *Context)
-    : ClangTidyCheck(Name, Context),
-      LineThreshold(Options.get("LineThreshold", -1U)),
-      StatementThreshold(Options.get("StatementThreshold", 800U)),
-      BranchThreshold(Options.get("BranchThreshold", -1U)) {}
+    : ClangTidyCheck(Name, Context), LineThreshold(-1U),
+      StatementThreshold(800U), BranchThreshold(-1U) {
+  StringRef Error = Options.parseOption("LineThreshold", LineThreshold);
+  if (!Error.empty())
+    llvm::errs() << "Invalid value for LineThreshold: " << Error << "\n";
+  Error = Options.parseOption("StatementThreshold", StatementThreshold);
+  if (!Error.empty())
+    llvm::errs() << "Invalid value for StatementThreshold: " << Error << "\n";
+  Error = Options.parseOption("BranchThreshold", BranchThreshold);
+  if (!Error.empty())
+    llvm::errs() << "Invalid value for BranchThreshold: " << Error << "\n";
+}
 
 void FunctionSizeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "LineThreshold", LineThreshold);
   Options.store(Opts, "StatementThreshold", StatementThreshold);
   Options.store(Opts, "BranchThreshold", BranchThreshold);
 }
 
 void FunctionSizeCheck::registerMatchers(MatchFinder *Finder) {

clang-tidy/readability/NamespaceCommentCheck.h

public:
   NamespaceCommentCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   void storeOptions(ClangTidyOptions::OptionMap &Options) override;
 
   llvm::Regex NamespaceCommentPattern;
-  const unsigned ShortNamespaceLines;
-  const unsigned SpacesBeforeComments;
+  unsigned ShortNamespaceLines;
+  unsigned SpacesBeforeComments;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NAMESPACECOMMENTCHECK_H

clang-tidy/readability/NamespaceCommentCheck.cpp

 namespace readability {
 
 NamespaceCommentCheck::NamespaceCommentCheck(StringRef Name,
                                              ClangTidyContext *Context)
     : 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)) {}
+      ShortNamespaceLines(1u), SpacesBeforeComments(1u) {
+  StringRef Error =
+      Options.parseOption("ShortNamespaceLines", ShortNamespaceLines);
+  if (!Error.empty())
+    llvm::errs() << "Invalid value for ShortNamespaceLines: " << Error << "\n";
+  Error = Options.parseOption("SpacesBeforeComments", SpacesBeforeComments);
+  if (!Error.empty())
+    llvm::errs() << "Invalid value for SpacesBeforeComments: " << Error << "\n";
+}
 
 void NamespaceCommentCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "ShortNamespaceLines", ShortNamespaceLines);
   Options.store(Opts, "SpacesBeforeComments", SpacesBeforeComments);
 }
 
 void NamespaceCommentCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(namespaceDecl().bind("namespace"), this);

unittests/clang-tidy/ClangTidyOptionsTest.cpp

+#include "ClangTidy.h"
 #include "ClangTidyOptions.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tidy {
 namespace test {
 
 TEST(ParseLineFilter, EmptyFilter) {
llvm::ErrorOr<ClangTidyOptions> Options =
                          "User: some.user");
   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.BooleanTrue\n"
+                         "    value: true\n"
+                         "  - key: test-check.BooleanFalse\n"
+                         "    value: false\n"
+                         );
+  EXPECT_TRUE(!!Options);
+  OptionsView Opt("test-check", Options->CheckOptions);
+  EXPECT_EQ("qqq", Opt.get("NonExistent", "qqq"));
+  EXPECT_EQ(" qwe !@#* ", Opt.get("String", ""));
+
+  EXPECT_FALSE(Opt.get("NonExistent").hasValue());
+  EXPECT_EQ(" qwe !@#* ", *Opt.get("String"));
+
+  int i = 123;
+  EXPECT_TRUE(Opt.parseOption("NonExistent", i).empty());
+  EXPECT_EQ(123, i);
+  EXPECT_TRUE(Opt.parseOption("IntegerPositive", i).empty());
+  EXPECT_EQ(12345, i);
+  EXPECT_TRUE(Opt.parseOption("IntegerNegative", i).empty());
+  EXPECT_EQ(-67890, i);
+  EXPECT_FALSE(Opt.parseOption("BooleanFalse", i).empty());
+  EXPECT_FALSE(Opt.parseOption("BooleanTrue", i).empty());
+  EXPECT_FALSE(Opt.parseOption("String", i).empty());
+
+  unsigned u = 123U;
+  EXPECT_TRUE(Opt.parseOption("NonExistent", u).empty());
+  EXPECT_EQ(123U, u);
+  EXPECT_TRUE(Opt.parseOption("IntegerPositive", u).empty());
+  EXPECT_EQ(12345U, u);
+  EXPECT_FALSE(Opt.parseOption("BooleanFalse", u).empty());
+  EXPECT_FALSE(Opt.parseOption("BooleanTrue", u).empty());
+  EXPECT_FALSE(Opt.parseOption("IntegerNegative", u).empty());
+  EXPECT_FALSE(Opt.parseOption("String", u).empty());
+
+  bool b = false;
+  EXPECT_TRUE(Opt.parseOption("NonExistent", b).empty());
+  EXPECT_FALSE(b);
+  EXPECT_TRUE(Opt.parseOption("BooleanTrue", b).empty());
+  EXPECT_TRUE(b);
+  EXPECT_TRUE(Opt.parseOption("BooleanFalse", b).empty());
+  EXPECT_FALSE(b);
+  EXPECT_FALSE(Opt.parseOption("IntegerNegative", b).empty());
+  EXPECT_FALSE(Opt.parseOption("IntegerPositive", b).empty());
+  EXPECT_FALSE(Opt.parseOption("String", b).empty());
+}
+
 } // namespace test
 } // namespace tidy
 } // namespace clang