[clang-tidy] Add support for optional parameters in config.

If a parameter value is either 'none', 'null', 'false', '-1'
or '', we will in that case use the default value.

Reviewed By: PiotrZSL

Differential Revision: https://reviews.llvm.org/D159436

Author: felix642

clang-tools-extra/clang-tidy/ClangTidyCheck.h

@@ -184,8 +184,8 @@ class ClangTidyCheck : public ast_matchers::MatchFinder::MatchCallback {
     /// integral type ``T``.
     ///
     /// Reads the option with the check-local name \p LocalName from the
-    /// ``CheckOptions``. If the corresponding key is not present, return
-    /// ``std::nullopt``.
+    /// ``CheckOptions``. If the corresponding key is not present,
+    ///  return ``std::nullopt``.
     ///
     /// If the corresponding key can't be parsed as a ``T``, emit a
     /// diagnostic and return ``std::nullopt``.
@@ -201,6 +201,31 @@ class ClangTidyCheck : public ast_matchers::MatchFinder::MatchCallback {
       return std::nullopt;
     }
 
+    /// Read a named option from the ``Context`` and parse it as an
+    /// integral type ``T``.
+    ///
+    /// Reads the option with the check-local name \p LocalName from the
+    /// ``CheckOptions``. If the corresponding key is `none`, `null`,
+    /// `-1` or empty, return ``std::nullopt``. If the corresponding
+    /// key is not present, return \p Default.
+    ///
+    /// If the corresponding key can't be parsed as a ``T``, emit a
+    /// diagnostic and return \p Default.
+    template <typename T>
+    std::enable_if_t<std::is_integral_v<T>, std::optional<T>>
+    get(StringRef LocalName, std::optional<T> Default) const {
+      if (std::optional<StringRef> Value = get(LocalName)) {
+        if (Value == "" || Value == "none" || Value == "null" ||
+            (std::is_unsigned_v<T> && Value == "-1"))
+          return std::nullopt;
+        T Result{};
+        if (!StringRef(*Value).getAsInteger(10, Result))
+          return Result;
+        diagnoseBadIntegerOption(NamePrefix + LocalName, *Value);
+      }
+      return Default;
+    }
+
     /// Read a named option from the ``Context`` and parse it as an
     /// integral type ``T``.
     ///
@@ -245,6 +270,39 @@ class ClangTidyCheck : public ast_matchers::MatchFinder::MatchCallback {
       return std::nullopt;
     }
 
+    /// Read a named option from the ``Context`` and parse it as an
+    /// integral type ``T``.
+    ///
+    /// Reads the option with the check-local name \p LocalName from local or
+    /// global ``CheckOptions``. Gets local option first. If local is not
+    /// present, falls back to get global option. If global option is not
+    /// present either, return \p Default. If the value value was found
+    /// and equals ``none``, ``null``, ``-1`` or empty, return ``std::nullopt``.
+    ///
+    /// If the corresponding key can't be parsed as a ``T``, emit a
+    /// diagnostic and return \p Default.
+    template <typename T>
+    std::enable_if_t<std::is_integral_v<T>, std::optional<T>>
+    getLocalOrGlobal(StringRef LocalName, std::optional<T> Default) const {
+      std::optional<StringRef> ValueOr = get(LocalName);
+      bool IsGlobal = false;
+      if (!ValueOr) {
+        IsGlobal = true;
+        ValueOr = getLocalOrGlobal(LocalName);
+        if (!ValueOr)
+          return Default;
+      }
+      T Result{};
+      if (ValueOr == "" || ValueOr == "none" || ValueOr == "null" ||
+          (std::is_unsigned_v<T> && ValueOr == "-1"))
+        return std::nullopt;
+      if (!StringRef(*ValueOr).getAsInteger(10, Result))
+        return Result;
+      diagnoseBadIntegerOption(
+          IsGlobal ? Twine(LocalName) : NamePrefix + LocalName, *ValueOr);
+      return Default;
+    }
+
     /// Read a named option from the ``Context`` and parse it as an
     /// integral type ``T``.
     ///
@@ -286,8 +344,8 @@ class ClangTidyCheck : public ast_matchers::MatchFinder::MatchCallback {
     /// enum type ``T``.
     ///
     /// Reads the option with the check-local name \p LocalName from the
-    /// ``CheckOptions``. If the corresponding key is not present, return
-    /// \p Default.
+    /// ``CheckOptions``. If the corresponding key is not present,
+    /// return \p Default.
     ///
     /// If the corresponding key can't be parsed as a ``T``, emit a
     /// diagnostic and return \p Default.
@@ -356,6 +414,19 @@ class ClangTidyCheck : public ast_matchers::MatchFinder::MatchCallback {
       storeInt(Options, LocalName, Value);
     }
 
+    /// Stores an option with the check-local name \p LocalName with
+    /// integer value \p Value to \p Options. If the value is empty
+    /// stores ``
+    template <typename T>
+    std::enable_if_t<std::is_integral_v<T>>
+    store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,
+          std::optional<T> Value) const {
+      if (Value)
+        storeInt(Options, LocalName, *Value);
+      else
+        store(Options, LocalName, "none");
+    }
+
     /// Stores an option with the check-local name \p LocalName as the string
     /// representation of the Enum \p Value to \p Options.
     ///

clang-tools-extra/clang-tidy/readability/FunctionSizeCheck.cpp

@@ -126,12 +126,16 @@ class FunctionASTVisitor : public RecursiveASTVisitor<FunctionASTVisitor> {
 
 FunctionSizeCheck::FunctionSizeCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
-      LineThreshold(Options.get("LineThreshold", -1U)),
-      StatementThreshold(Options.get("StatementThreshold", 800U)),
-      BranchThreshold(Options.get("BranchThreshold", -1U)),
-      ParameterThreshold(Options.get("ParameterThreshold", -1U)),
-      NestingThreshold(Options.get("NestingThreshold", -1U)),
-      VariableThreshold(Options.get("VariableThreshold", -1U)) {}
+      LineThreshold(Options.get("LineThreshold", DefaultLineThreshold)),
+      StatementThreshold(
+          Options.get("StatementThreshold", DefaultStatementThreshold)),
+      BranchThreshold(Options.get("BranchThreshold", DefaultBranchThreshold)),
+      ParameterThreshold(
+          Options.get("ParameterThreshold", DefaultParameterThreshold)),
+      NestingThreshold(
+          Options.get("NestingThreshold", DefaultNestingThreshold)),
+      VariableThreshold(
+          Options.get("VariableThreshold", DefaultVariableThreshold)) {}
 
 void FunctionSizeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "LineThreshold", LineThreshold);
@@ -155,7 +159,7 @@ void FunctionSizeCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Func = Result.Nodes.getNodeAs<FunctionDecl>("func");
 
   FunctionASTVisitor Visitor;
-  Visitor.Info.NestingThreshold = NestingThreshold;
+  Visitor.Info.NestingThreshold = NestingThreshold.value_or(-1);
   Visitor.TraverseDecl(const_cast<FunctionDecl *>(Func));
   auto &FI = Visitor.Info;
 
@@ -173,49 +177,51 @@ void FunctionSizeCheck::check(const MatchFinder::MatchResult &Result) {
 
   unsigned ActualNumberParameters = Func->getNumParams();
 
-  if (FI.Lines > LineThreshold || FI.Statements > StatementThreshold ||
-      FI.Branches > BranchThreshold ||
-      ActualNumberParameters > ParameterThreshold ||
-      !FI.NestingThresholders.empty() || FI.Variables > VariableThreshold) {
+  if ((LineThreshold && FI.Lines > LineThreshold) ||
+      (StatementThreshold && FI.Statements > StatementThreshold) ||
+      (BranchThreshold && FI.Branches > BranchThreshold) ||
+      (ParameterThreshold && ActualNumberParameters > ParameterThreshold) ||
+      !FI.NestingThresholders.empty() ||
+      (VariableThreshold && FI.Variables > VariableThreshold)) {
     diag(Func->getLocation(),
          "function %0 exceeds recommended size/complexity thresholds")
         << Func;
   }
 
-  if (FI.Lines > LineThreshold) {
+  if (LineThreshold && FI.Lines > LineThreshold) {
     diag(Func->getLocation(),
          "%0 lines including whitespace and comments (threshold %1)",
          DiagnosticIDs::Note)
-        << FI.Lines << LineThreshold;
+        << FI.Lines << LineThreshold.value();
   }
 
-  if (FI.Statements > StatementThreshold) {
+  if (StatementThreshold && FI.Statements > StatementThreshold) {
     diag(Func->getLocation(), "%0 statements (threshold %1)",
          DiagnosticIDs::Note)
-        << FI.Statements << StatementThreshold;
+        << FI.Statements << StatementThreshold.value();
   }
 
-  if (FI.Branches > BranchThreshold) {
+  if (BranchThreshold && FI.Branches > BranchThreshold) {
     diag(Func->getLocation(), "%0 branches (threshold %1)", DiagnosticIDs::Note)
-        << FI.Branches << BranchThreshold;
+        << FI.Branches << BranchThreshold.value();
   }
 
-  if (ActualNumberParameters > ParameterThreshold) {
+  if (ParameterThreshold && ActualNumberParameters > ParameterThreshold) {
     diag(Func->getLocation(), "%0 parameters (threshold %1)",
          DiagnosticIDs::Note)
-        << ActualNumberParameters << ParameterThreshold;
+        << ActualNumberParameters << ParameterThreshold.value();
   }
 
   for (const auto &CSPos : FI.NestingThresholders) {
     diag(CSPos, "nesting level %0 starts here (threshold %1)",
          DiagnosticIDs::Note)
-        << NestingThreshold + 1 << NestingThreshold;
+        << NestingThreshold.value() + 1 << NestingThreshold.value();
   }
 
-  if (FI.Variables > VariableThreshold) {
+  if (VariableThreshold && FI.Variables > VariableThreshold) {
     diag(Func->getLocation(), "%0 variables (threshold %1)",
          DiagnosticIDs::Note)
-        << FI.Variables << VariableThreshold;
+        << FI.Variables << VariableThreshold.value();
   }
 }
 

clang-tools-extra/clang-tidy/readability/FunctionSizeCheck.h

@@ -41,12 +41,23 @@ class FunctionSizeCheck : public ClangTidyCheck {
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
-  const unsigned LineThreshold;
-  const unsigned StatementThreshold;
-  const unsigned BranchThreshold;
-  const unsigned ParameterThreshold;
-  const unsigned NestingThreshold;
-  const unsigned VariableThreshold;
+  const std::optional<unsigned> LineThreshold;
+  const std::optional<unsigned> StatementThreshold;
+  const std::optional<unsigned> BranchThreshold;
+  const std::optional<unsigned> ParameterThreshold;
+  const std::optional<unsigned> NestingThreshold;
+  const std::optional<unsigned> VariableThreshold;
+
+  static constexpr std::optional<unsigned> DefaultLineThreshold = std::nullopt;
+  static constexpr std::optional<unsigned> DefaultStatementThreshold = 800U;
+  static constexpr std::optional<unsigned> DefaultBranchThreshold =
+      std::nullopt;
+  static constexpr std::optional<unsigned> DefaultParameterThreshold =
+      std::nullopt;
+  static constexpr std::optional<unsigned> DefaultNestingThreshold =
+      std::nullopt;
+  static constexpr std::optional<unsigned> DefaultVariableThreshold =
+      std::nullopt;
 };
 
 } // namespace clang::tidy::readability

clang-tools-extra/docs/ReleaseNotes.rst

@@ -312,6 +312,10 @@ Changes in existing checks
   detect comparison between string and empty string literals and support
   ``length()`` method as an alternative to ``size()``.
 
+- Improved :doc:`readability-function-size
+  <clang-tidy/checks/readability/function-size>` check configuration to use
+  `none` rather than `-1` to disable some parameters.
+
 - Improved :doc:`readability-identifier-naming
   <clang-tidy/checks/readability/identifier-naming>` check to issue accurate
   warnings when a type's forward declaration precedes its definition.
@@ -334,7 +338,6 @@ Changes in existing checks
   <clang-tidy/checks/readability/static-accessed-through-instance>` check to
   identify calls to static member functions with out-of-class inline definitions.
 
-
 Removed checks
 ^^^^^^^^^^^^^^
 

clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst

@@ -12,7 +12,7 @@ Options
 
 .. option:: LineThreshold
 
-   Flag functions exceeding this number of lines. The default is `-1` (ignore
+   Flag functions exceeding this number of lines. The default is `none` (ignore
    the number of lines).
 
 .. option:: StatementThreshold
@@ -24,22 +24,22 @@ Options
 .. option:: BranchThreshold
 
    Flag functions exceeding this number of control statements. The default is
-   `-1` (ignore the number of branches).
+   `none` (ignore the number of branches).
 
 .. option:: ParameterThreshold
 
    Flag functions that exceed a specified number of parameters. The default
-   is `-1` (ignore the number of parameters).
+   is `none` (ignore the number of parameters).
 
 .. option:: NestingThreshold
 
     Flag compound statements which create next nesting level after
     `NestingThreshold`. This may differ significantly from the expected value
-    for macro-heavy code. The default is `-1` (ignore the nesting level).
+    for macro-heavy code. The default is `none` (ignore the nesting level).
 
 .. option:: VariableThreshold
 
    Flag functions exceeding this number of variables declared in the body.
-   The default is `-1` (ignore the number of variables).
+   The default is `none` (ignore the number of variables).
    Please note that function parameters and variables declared in lambdas,
    GNU Statement Expressions, and nested class inline functions are not counted.

clang-tools-extra/test/clang-tidy/checkers/readability/function-size.cpp

@@ -8,6 +8,16 @@
 // RUN:         readability-function-size.VariableThreshold: 1 \
 // RUN:     }}'
 
+
+// RUN: %check_clang_tidy -check-suffixes=OPTIONAL %s readability-function-size %t -- \
+// RUN:     -config='{CheckOptions: { \
+// RUN:         readability-function-size.StatementThreshold: "-1", \
+// RUN:         readability-function-size.BranchThreshold: "5", \
+// RUN:         readability-function-size.ParameterThreshold: "none", \
+// RUN:         readability-function-size.NestingThreshold: "", \
+// RUN:         readability-function-size.VariableThreshold: "" \
+// RUN:     }}'
+
 // Bad formatting is intentional, don't run clang-format over the whole file!
 
 void foo1() {
@@ -103,9 +113,11 @@ void baz0() { // 1
 // check that nested if's are not reported. this was broken initially
 void nesting_if() { // 1
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'nesting_if' exceeds recommended size/complexity
-  // CHECK-MESSAGES: :[[@LINE-2]]:6: note: 23 lines including whitespace and comments (threshold 0)
+  // CHECK-MESSAGES: :[[@LINE-2]]:6: note: 25 lines including whitespace and comments (threshold 0)
   // CHECK-MESSAGES: :[[@LINE-3]]:6: note: 18 statements (threshold 0)
   // CHECK-MESSAGES: :[[@LINE-4]]:6: note: 6 branches (threshold 0)
+  // CHECK-MESSAGES-OPTIONAL: :[[@LINE-5]]:6: warning: function 'nesting_if' exceeds recommended size/complexity
+  // CHECK-MESSAGES-OPTIONAL: :[[@LINE-6]]:6: note: 6 branches (threshold 5)
   if (true) { // 2
      int j;
   } else if (true) { // 2
@@ -123,7 +135,7 @@ void nesting_if() { // 1
   } else if (true) { // 2
      int j;
   }
-  // CHECK-MESSAGES: :[[@LINE-22]]:6: note: 6 variables (threshold 1)
+  // CHECK-MESSAGES: :[[@LINE-24]]:6: note: 6 variables (threshold 1)
 }
 
 // however this should warn

clang-tools-extra/test/clang-tidy/infrastructure/config-files.cpp

@@ -55,5 +55,12 @@
 // CHECK-CHILD6: Checks: {{.*-llvm-qualified-auto'? *$}}
 // CHECK-CHILD6-NOT: modernize-use-using.IgnoreMacros
 
+// RUN: clang-tidy -dump-config \
+// RUN: --config='{CheckOptions: {readability-function-size.LineThreshold: ""}, \
+// RUN: Checks: readability-function-size}' \
+// RUN: %S/Inputs/config-files/4/44/- -- | FileCheck %s -check-prefix=CHECK-CHILD7
+// CHECK-CHILD7: readability-function-size.LineThreshold: none
+
+
 // Validate that check options are printed in alphabetical order:
 // RUN: clang-tidy --checks="-*,readability-identifier-naming" --dump-config %S/Inputs/config-files/- -- | grep "readability-identifier-naming\." | sort --check