diff --git a/clang-tools-extra/clang-tidy/ClangTidyCheck.h b/clang-tools-extra/clang-tidy/ClangTidyCheck.h index 39b9f9fa865b7..656a2f008f6e0 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyCheck.h +++ b/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 + std::enable_if_t, std::optional> + get(StringRef LocalName, std::optional Default) const { + if (std::optional Value = get(LocalName)) { + if (Value == "" || Value == "none" || Value == "null" || + (std::is_unsigned_v && 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 + std::enable_if_t, std::optional> + getLocalOrGlobal(StringRef LocalName, std::optional Default) const { + std::optional 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 && 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 + std::enable_if_t> + store(ClangTidyOptions::OptionMap &Options, StringRef LocalName, + std::optional 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. /// diff --git a/clang-tools-extra/clang-tidy/readability/FunctionSizeCheck.cpp b/clang-tools-extra/clang-tidy/readability/FunctionSizeCheck.cpp index 3c803959caf80..3313bcb39b7f3 100644 --- a/clang-tools-extra/clang-tidy/readability/FunctionSizeCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/FunctionSizeCheck.cpp @@ -126,12 +126,16 @@ class FunctionASTVisitor : public RecursiveASTVisitor { 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("func"); FunctionASTVisitor Visitor; - Visitor.Info.NestingThreshold = NestingThreshold; + Visitor.Info.NestingThreshold = NestingThreshold.value_or(-1); Visitor.TraverseDecl(const_cast(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(); } } diff --git a/clang-tools-extra/clang-tidy/readability/FunctionSizeCheck.h b/clang-tools-extra/clang-tidy/readability/FunctionSizeCheck.h index 5f3fb9a35dd54..106c69ff07393 100644 --- a/clang-tools-extra/clang-tidy/readability/FunctionSizeCheck.h +++ b/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 LineThreshold; + const std::optional StatementThreshold; + const std::optional BranchThreshold; + const std::optional ParameterThreshold; + const std::optional NestingThreshold; + const std::optional VariableThreshold; + + static constexpr std::optional DefaultLineThreshold = std::nullopt; + static constexpr std::optional DefaultStatementThreshold = 800U; + static constexpr std::optional DefaultBranchThreshold = + std::nullopt; + static constexpr std::optional DefaultParameterThreshold = + std::nullopt; + static constexpr std::optional DefaultNestingThreshold = + std::nullopt; + static constexpr std::optional DefaultVariableThreshold = + std::nullopt; }; } // namespace clang::tidy::readability diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 60b82f4c50dd7..03e5dc6f164af 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/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 + ` check configuration to use + `none` rather than `-1` to disable some parameters. + - Improved :doc:`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 ` check to identify calls to static member functions with out-of-class inline definitions. - Removed checks ^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst index 3360fbd5f1e63..133bd3e9c8cbe 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst +++ b/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. diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/function-size.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/function-size.cpp index b6a79465d981c..45b2604b43d03 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/function-size.cpp +++ b/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 diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/config-files.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/config-files.cpp index cc2dbc464e2d7..ab4f3becb7a9f 100644 --- a/clang-tools-extra/test/clang-tidy/infrastructure/config-files.cpp +++ b/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