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

[felix642]

Summary

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

[felix642]

Added entry to release notes

[felix642]

Reworded release notes

[carlosgalvezp]

Thanks for the patch! I must admit I don't fully understand what problem it solves, i.e. parameters are already optional today (one just simply doesn't specify them in the config file). Why would we want to explicitly spell out parameters with some default value that doesn't correspond to what actually gets applied in the check?

[carlosgalvezp]

clang-tools-extra/test/clang-tidy/infrastructure/optional-parameter.cpp

19

What if:

• "MinimumLength" is a boolean.
• It's default value (if not specified) is True.
• And a user wants to set it as "False" here

Wouldn't that cause problems?

[PiotrZSL]

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

196–198

To be honest I wanted this to work only if we call get with Default being std::optional<T>, and such method should return optional, and store methods should be also updated.
And next time use github pull requests.

clang-tools-extra/test/clang-tidy/infrastructure/optional-parameter.cpp

19

this functionality should work only for integers...
Idea is to have:

template <typename T>
std::enable_if_t<std::is_integral_v<T>, std::optional<T>>
getLocalOrGlobal(StringRef LocalName, std::optional<T> Default) const;

 template <typename T>
 std::enable_if_t<std::is_integral_v<T>, std::optional<T>>
 get(StringRef LocalName, std::optional<T> Default) const;

without impacting other functions... so that instead of doing tricks with -1 in code, we could simply create real optional options, and be able to 'store' them as optional, this mean that --dump-config should dump them as "none".

[felix642]

Updated Differential to truly support optional parameters

[felix642]

Updated documentation

[felix642]

Removed false option

[felix642]

Moved to a new method, changed tests, changed documentation

[felix642]

Hi @PiotrZSL and @carlosgalvezp, I have updated my diff based on your comments. Let me know what you think.

What if:

"MinimumLength" is a boolean.
It's default value (if not specified) is True.
And a user wants to set it as "False" here

Wouldn't that cause problems?

I have removed the "false" option to support optional for boolean types.

this functionality should work only for integers...
Idea is to have:

template <typename T>
std::enable_if_t<std::is_integral_v<T>, std::optional<T>>
getLocalOrGlobal(StringRef LocalName, std::optional<T> Default) const;

 template <typename T>
 std::enable_if_t<std::is_integral_v<T>, std::optional<T>>
 get(StringRef LocalName, std::optional<T> Default) const;

without impacting other functions... so that instead of doing tricks with -1 in code, we could simply create real optional options, and be able to 'store' them as optional, this mean that --dump-config should dump them as "none".

I have moved the declaration to a new method which works only on integral types where the default value is std::optional. Let me know what you think.

[felix642]

Forgot to update a comment

[felix642]

ping.

[PiotrZSL]

Except few nits in documentation looks fine. Probably one day in future we can add same for bools.
Make sure that documentation of impacted checks is correct, so it says that argument is positive integer, or none to force default value.

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

20–34

Those options documentation could be removed from this file. Only keep check description.

clang-tools-extra/docs/ReleaseNotes.rst

116–118

I'm not sure if this release notes entry is needed. Maybe this change should be documented per changed check basic.

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

15–16

Leave previous but change -1 into none.

[felix642]

Updated documentation

[felix642]

Wrong check name

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

public:
     /// present, falls back to get global option. If global option is not
     /// present either, returns \p Default.
     StringRef getLocalOrGlobal(StringRef LocalName, StringRef Default) const;
 
     /// 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 not present, return
-    /// ``std::nullopt``.
+    /// ``CheckOptions``. If the corresponding key is not present or empty,
+    ///  return ``std::nullopt``.
     ///
     /// If the corresponding key can't be parsed as a ``T``, emit a
     /// diagnostic and return ``std::nullopt``.
     template <typename T>
     std::enable_if_t<std::is_integral_v<T>, std::optional<T>>
     get(StringRef LocalName) const {
       if (std::optional<StringRef> Value = get(LocalName)) {
+        if (Value == "" || Value == "none" || Value == "null" ||
+            Value == "false" || (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 std::nullopt;
     }
 
getLocalOrGlobal(StringRef LocalName) const {
       bool IsGlobal = false;
       if (!ValueOr) {
         IsGlobal = true;
         ValueOr = getLocalOrGlobal(LocalName);
         if (!ValueOr)
           return std::nullopt;
       }
       T Result{};
+      if (ValueOr == "" || ValueOr == "none" || ValueOr == "null" ||
+          ValueOr == "false" || (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 std::nullopt;
     }
 
     /// Read a named option from the ``Context`` and parse it as an
get(StringRef LocalName, bool IgnoreCase = false) const {
         return static_cast<T>(*ValueOr);
       return std::nullopt;
     }
 
     /// Read a named option from the ``Context`` and parse it as an
     /// 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 or empty,
+    /// return \p Default.
     ///
     /// If the corresponding key can't be parsed as a ``T``, emit a
     /// diagnostic and return \p Default.
     ///
     /// \ref clang::tidy::OptionEnumMapping must be specialized for ``T`` to
     /// supply the mapping required to convert between ``T`` and a string.
     template <typename T>
     std::enable_if_t<std::is_enum_v<T>, T> get(StringRef LocalName, T Default,
public:
     /// integer value \p Value to \p Options.
     template <typename T>
     std::enable_if_t<std::is_integral_v<T>>
     store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,
           T Value) const {
       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.
     ///
     /// \ref clang::tidy::OptionEnumMapping must be specialized for ``T`` to
     /// supply the mapping required to convert between ``T`` and a string.
     template <typename T>
     std::enable_if_t<std::is_enum_v<T>>
     store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,

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

 #include "../ClangTidyCheck.h"
 
 namespace clang::tidy::readability {
 
 /// Checks for large functions based on various metrics.
 ///
 /// These options are supported:
 ///
-///   * `LineThreshold` - flag functions exceeding this number of lines. The
-///     default is `-1` (ignore the number of lines).
+///   * `LineThreshold` - flag functions exceeding this number of lines. This
+///     parameter is disabled by default.
 ///   * `StatementThreshold` - flag functions exceeding this number of
 ///     statements. This may differ significantly from the number of lines for
 ///     macro-heavy code. The default is `800`.
 ///   * `BranchThreshold` - flag functions exceeding this number of control
-///     statements. The default is `-1` (ignore the number of branches).
+///     statements. This parameter is disabled by default.
 ///   * `ParameterThreshold` - flag functions having a high number of
-///     parameters. The default is `-1` (ignore the number of parameters).
+///     parameters. This parameter is disabled by default.
 ///   * `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).
+///     expected value for macro-heavy code. This parameter is disabled by
+///     default.
 ///   * `VariableThreshold` - flag functions having a high number of variable
-///     declarations. The default is `-1` (ignore the number of variables).
+///     declarations. This parameter is disabled by default.
 class FunctionSizeCheck : public ClangTidyCheck {
 public:
   FunctionSizeCheck(StringRef Name, ClangTidyContext *Context);
 
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   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;
 };
 
 } // namespace clang::tidy::readability
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_FUNCTIONSIZECHECK_H

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

public:
   unsigned StructNesting = 0;
   unsigned CurrentNestingLevel = 0;
 };
 
 } // namespace
 
 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<unsigned>("LineThreshold")),
+      StatementThreshold(Options.get<unsigned>("StatementThreshold", 800U)),
+      BranchThreshold(Options.get<unsigned>("BranchThreshold")),
+      ParameterThreshold(Options.get<unsigned>("ParameterThreshold")),
+      NestingThreshold(Options.get<unsigned>("NestingThreshold")),
+      VariableThreshold(Options.get<unsigned>("VariableThreshold")) {}
 
 void FunctionSizeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "LineThreshold", LineThreshold);
   Options.store(Opts, "StatementThreshold", StatementThreshold);
   Options.store(Opts, "BranchThreshold", BranchThreshold);
   Options.store(Opts, "ParameterThreshold", ParameterThreshold);
   Options.store(Opts, "NestingThreshold", NestingThreshold);
   Options.store(Opts, "VariableThreshold", VariableThreshold);
 }
 
 void FunctionSizeCheck::registerMatchers(MatchFinder *Finder) {
   // Lambdas ignored - historically considered part of enclosing function.
   // FIXME: include them instead? Top-level lambdas are currently never counted.
   Finder->addMatcher(functionDecl(unless(isInstantiated()),
                                   unless(cxxMethodDecl(ofClass(isLambda()))))
                          .bind("func"),
                      this);
 }
 
 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;
 
   if (FI.Statements == 0)
     return;
 
   // Count the lines including whitespace and comments. Really simple.
   if (const Stmt *Body = Func->getBody()) {
     SourceManager *SM = Result.SourceManager;
     if (SM->isWrittenInSameFile(Body->getBeginLoc(), Body->getEndLoc())) {
       FI.Lines = SM->getSpellingLineNumber(Body->getEndLoc()) -
                  SM->getSpellingLineNumber(Body->getBeginLoc());
     }
   }
 
   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();
   }
 }
 
 } // namespace clang::tidy::readability

clang-tools-extra/docs/ReleaseNotes.rst

- Preprocessor-level module header parsing is now disabled by default due to
   functionality using the newly added command line option
   `--enable-module-headers-parsing`.
 
 - Remove configuration option `AnalyzeTemporaryDestructors`, which was deprecated since
   :program:`clang-tidy` 16.
 
 - Improved `--dump-config` to print check options in alphabetical order.
 
+- Added support for optional parameters. Parameters that previously used -1 to disable
+  their effect can now be set to `none`, `null`, `false`, or left empty to ge the same
+  behaviour.
+
 New checks
 ^^^^^^^^^^
 
 - New :doc:`bugprone-inc-dec-in-conditions
   <clang-tidy/checks/bugprone/inc-dec-in-conditions>` check.
 
   Detects when a variable is both incremented/decremented and referenced inside
   a complex condition and suggests moving them outside to avoid ambiguity in

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

 .. title:: clang-tidy - readability-function-size
 
 readability-function-size
 =========================
 
 `google-readability-function-size` redirects here as an alias for this check.
 
 Checks for large functions based on various metrics.
 
 Options
 -------
 
 .. option:: LineThreshold
 
-   Flag functions exceeding this number of lines. The default is `-1` (ignore
-   the number of lines).
+   Flag functions exceeding this number of lines. This parameter is disabled
+   by default.
 
 .. option:: StatementThreshold
 
    Flag functions exceeding this number of statements. This may differ
    significantly from the number of lines for macro-heavy code. The default is
    `800`.
 
 .. option:: BranchThreshold
 
-   Flag functions exceeding this number of control statements. The default is
-   `-1` (ignore the number of branches).
+   Flag functions exceeding this number of control statements. This parameter
+   is disabled by default.
 
 .. option:: ParameterThreshold
 
-   Flag functions that exceed a specified number of parameters. The default
-   is `-1` (ignore the number of parameters).
+   Flag functions that exceed a specified number of parameters. This parameter
+   is disabled by default.
 
 .. 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. This parameter is disabled by default.
 
 .. option:: VariableThreshold
 
    Flag functions exceeding this number of variables declared in the body.
-   The default is `-1` (ignore the number of variables).
+   This parameter is disabled by default.
    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/infrastructure/optional-parameter.cpp

+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: { \
+// RUN:     bugprone-easily-swappable-parameters.MinimumLength: "", \
+// RUN:  }}' --
+
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: { \
+// RUN:     bugprone-easily-swappable-parameters.MinimumLength: "none", \
+// RUN:  }}' --
+
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: { \
+// RUN:     bugprone-easily-swappable-parameters.MinimumLength: "null", \
+// RUN:  }}' --
+
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: { \
+// RUN:     bugprone-easily-swappable-parameters.MinimumLength: "false", \
+// RUN:  }}' --
+
+void a(int b, int c) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: 2 adjacent parameters of 'a' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:12: note: the first parameter in the range is 'b'
+// CHECK-MESSAGES: :[[@LINE-3]]:19: note: the last parameter in the range is 'c'