Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Differential D44602 Diff 139711 clang-tidy/readability/FunctionSizeCheck.cpp

Changeset View

Standalone View

View Options

clang-tidy/readability/FunctionSizeCheck.cpp

Show All 16 Lines
namespace tidy { namespace tidy {
namespace readability { namespace readability {
namespace { namespace {
class FunctionASTVisitor : public RecursiveASTVisitor<FunctionASTVisitor> { class FunctionASTVisitor : public RecursiveASTVisitor<FunctionASTVisitor> {
using Base = RecursiveASTVisitor<FunctionASTVisitor>; using Base = RecursiveASTVisitor<FunctionASTVisitor>;
public: public:
bool VisitVarDecl(VarDecl *VD) {
// Do not count function params.
// Do not count decomposition declarations (C++17's structured bindings).
// Do not count variables declared in macros.
aaron.ballmanUnsubmitted
Done
ReplyInline Actions

Comment is now stale.

aaron.ballman: Comment is now stale.
if (StructNesting == 0 &&
!(isa<ParmVarDecl>(VD) || isa<DecompositionDecl>(VD)) &&
!VD->getLocation().isMacroID())
aaron.ballmanUnsubmitted
Done
ReplyInline Actions

This comment could be clarified. I think what it's trying to express is that the decomposition declaration was already counted as a variable declaration but we do not want to count it as such.

I think a better way to express this is to change VisitVarDecl() to check isa<DecompositionDecl>() and not increment in that case, rather than increment in one place and decrement in another. You could also check isa<ParmVarDecl>() at the same time and skip subtracting off formal parameters.

aaron.ballman: This comment could be clarified. I think what it's trying to express is that the decomposition…
lebedev.riAuthorUnsubmitted
Done
ReplyInline Actions

This comment could be clarified. I think what it's trying to express is that the decomposition declaration was already counted as a variable declaration but we do not want to count it as such.

Yep.

I think a better way to express this is to change VisitVarDecl() to check isa<DecompositionDecl>() and not increment in that case, rather than increment in one place and decrement in another.

I agree.

You could also check isa<ParmVarDecl>() at the same time and skip subtracting off formal parameters.

That is a behavior-changing change, actually.
But i don't have any strong preferences here, so changed, test added.

lebedev.ri: > This comment could be clarified. I think what it's trying to express is that the…
aaron.ballmanUnsubmitted
Done
ReplyInline Actions

This isn't the restriction I was envisioning. I was thinking more along the lines of:

bool VisitStmtExpr(StmtExpr *SE) override {
  ++StructNesting;
  Base::VisitStmtExpr(SE);
  --StructNesting;
  return true;
}

Basically -- treat a statement expression the same as an inner struct or lambda because the variables declared within it are scoped *only* to the statement expression.

You could argue that we should also add: if (!SE->getLocation.isMacroID()) { Base::VisitStmtExpr(SE); } to the top of the function so that you only treat statement expressions that are themselves in a macro expansion get this special treatment. e.g.,

void one_var() {
  (void)({int a = 12; a;});
}

#define M ({int a = 12; a;})
void zero_var() {
  (void)M;
}

I don't have strong opinions on this, but weakly lean towards treating all statement expressions the same way.

aaron.ballman: This isn't the restriction I was envisioning. I was thinking more along the lines of: ``` bool…
lebedev.riAuthorUnsubmitted
Not Done
ReplyInline Actions

Ah, i did not really knew about GNU Statement Expression, so i did not ignore them.
Test added, and ignored.

lebedev.ri: Ah, i did not really knew about `GNU Statement Expression`, so i did not ignore them. Test…
aaron.ballmanUnsubmitted
Done
ReplyInline Actions

Do you mind switching these to preincrement (and predecrement, below) since the value is not subsequently used?

aaron.ballman: Do you mind switching these to preincrement (and predecrement, below) since the value is not…
Info.Variables++;
return true;
}
bool VisitBindingDecl(BindingDecl *BD) {
// Do count each of the bindings (in the decomposition declaration).
if (StructNesting == 0 && !BD->getLocation().isMacroID())
Info.Variables++;
return true;
}
bool TraverseStmt(Stmt *Node) { bool TraverseStmt(Stmt *Node) {
if (!Node) if (!Node)
return Base::TraverseStmt(Node); return Base::TraverseStmt(Node);
if (TrackedParent.back() && !isa<CompoundStmt>(Node)) if (TrackedParent.back() && !isa<CompoundStmt>(Node))
++Info.Statements; ++Info.Statements;
switch (Node->getStmtClass()) { switch (Node->getStmtClass()) {
Show All 36 Linespublic:
bool TraverseDecl(Decl *Node) { bool TraverseDecl(Decl *Node) {
TrackedParent.push_back(false); TrackedParent.push_back(false);
Base::TraverseDecl(Node); Base::TraverseDecl(Node);
TrackedParent.pop_back(); TrackedParent.pop_back();
return true; return true;
} }
bool TraverseLambdaExpr(LambdaExpr *Node) {
StructNesting++;
Base::TraverseLambdaExpr(Node);
StructNesting--;
return true;
}
bool TraverseCXXRecordDecl(CXXRecordDecl *Node) {
StructNesting++;
Base::TraverseCXXRecordDecl(Node);
StructNesting--;
return true;
}
struct FunctionInfo { struct FunctionInfo {
unsigned Lines = 0; unsigned Lines = 0;
unsigned Statements = 0; unsigned Statements = 0;
unsigned Branches = 0; unsigned Branches = 0;
unsigned NestingThreshold = 0; unsigned NestingThreshold = 0;
unsigned Variables = 0;
std::vector<SourceLocation> NestingThresholders; std::vector<SourceLocation> NestingThresholders;
}; };
FunctionInfo Info; FunctionInfo Info;
std::vector<bool> TrackedParent; std::vector<bool> TrackedParent;
unsigned StructNesting = 0;
unsigned CurrentNestingLevel = 0; unsigned CurrentNestingLevel = 0;
}; };
} // namespace } // namespace
FunctionSizeCheck::FunctionSizeCheck(StringRef Name, ClangTidyContext *Context) FunctionSizeCheck::FunctionSizeCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context), : ClangTidyCheck(Name, Context),
LineThreshold(Options.get("LineThreshold", -1U)), LineThreshold(Options.get("LineThreshold", -1U)),
StatementThreshold(Options.get("StatementThreshold", 800U)), StatementThreshold(Options.get("StatementThreshold", 800U)),
BranchThreshold(Options.get("BranchThreshold", -1U)), BranchThreshold(Options.get("BranchThreshold", -1U)),
ParameterThreshold(Options.get("ParameterThreshold", -1U)), ParameterThreshold(Options.get("ParameterThreshold", -1U)),
NestingThreshold(Options.get("NestingThreshold", -1U)) {} NestingThreshold(Options.get("NestingThreshold", -1U)),
VariableThreshold(Options.get("VariableThreshold", -1U)) {}
void FunctionSizeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { void FunctionSizeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "LineThreshold", LineThreshold); Options.store(Opts, "LineThreshold", LineThreshold);
Options.store(Opts, "StatementThreshold", StatementThreshold); Options.store(Opts, "StatementThreshold", StatementThreshold);
Options.store(Opts, "BranchThreshold", BranchThreshold); Options.store(Opts, "BranchThreshold", BranchThreshold);
Options.store(Opts, "ParameterThreshold", ParameterThreshold); Options.store(Opts, "ParameterThreshold", ParameterThreshold);
Options.store(Opts, "NestingThreshold", NestingThreshold); Options.store(Opts, "NestingThreshold", NestingThreshold);
Options.store(Opts, "VariableThreshold", VariableThreshold);
} }
void FunctionSizeCheck::registerMatchers(MatchFinder *Finder) { void FunctionSizeCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(functionDecl(unless(isInstantiated())).bind("func"), this); Finder->addMatcher(functionDecl(unless(isInstantiated())).bind("func"), this);
} }
void FunctionSizeCheck::check(const MatchFinder::MatchResult &Result) { void FunctionSizeCheck::check(const MatchFinder::MatchResult &Result) {
const auto *Func = Result.Nodes.getNodeAs<FunctionDecl>("func"); const auto *Func = Result.Nodes.getNodeAs<FunctionDecl>("func");
Show All 15 Lines if (const Stmt *Body = Func->getBody()) {
} }
} }
unsigned ActualNumberParameters = Func->getNumParams(); unsigned ActualNumberParameters = Func->getNumParams();
if (FI.Lines > LineThreshold || FI.Statements > StatementThreshold || if (FI.Lines > LineThreshold || FI.Statements > StatementThreshold ||
FI.Branches > BranchThreshold || FI.Branches > BranchThreshold ||
ActualNumberParameters > ParameterThreshold || ActualNumberParameters > ParameterThreshold ||
!FI.NestingThresholders.empty()) { !FI.NestingThresholders.empty() || FI.Variables > VariableThreshold) {
diag(Func->getLocation(), diag(Func->getLocation(),
"function %0 exceeds recommended size/complexity thresholds") "function %0 exceeds recommended size/complexity thresholds")
<< Func; << Func;
} }
if (FI.Lines > LineThreshold) { if (FI.Lines > LineThreshold) {
diag(Func->getLocation(), diag(Func->getLocation(),
"%0 lines including whitespace and comments (threshold %1)", "%0 lines including whitespace and comments (threshold %1)",
Show All 18 Lines diag(Func->getLocation(), "%0 parameters (threshold %1)",
<< ActualNumberParameters << ParameterThreshold; << ActualNumberParameters << ParameterThreshold;
} }
for (const auto &CSPos : FI.NestingThresholders) { for (const auto &CSPos : FI.NestingThresholders) {
diag(CSPos, "nesting level %0 starts here (threshold %1)", diag(CSPos, "nesting level %0 starts here (threshold %1)",
DiagnosticIDs::Note) DiagnosticIDs::Note)
<< NestingThreshold + 1 << NestingThreshold; << NestingThreshold + 1 << NestingThreshold;
} }
if (FI.Variables > VariableThreshold) {
diag(Func->getLocation(), "%0 variables (threshold %1)",
DiagnosticIDs::Note)
<< FI.Variables << VariableThreshold;
}
} }
} // namespace readability } // namespace readability
} // namespace tidy } // namespace tidy
} // namespace clang } // namespace clang