steveire updated this revision to Diff 313346.
steveire added a comment.
Update
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91302/new/
https://reviews.llvm.org/D91302
Files:
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp
@@ -483,3 +483,192 @@
f<double>();
f<char *>();
}
+
+template <typename T>
+bool neverInstantiatedTemplate() {
+ std::vector<T> v;
+ if (v.size())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
+ // CHECK-MESSAGES: :9:8: note: method 'vector'::empty() defined here
+ // CHECK-FIXES: {{^ }}if (!v.empty()){{$}}
+
+ if (v == std::vector<T>())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of comparing to an empty object [readability-container-size-empty]
+ // CHECK-FIXES: {{^ }}if (v.empty()){{$}}
+ // CHECK-FIXES-NEXT: ;
+ if (v.size() == 0)
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
+ // CHECK-MESSAGES: :9:8: note: method 'vector'::empty() defined here
+ // CHECK-FIXES: {{^ }}if (v.empty()){{$}}
+ if (static_cast<bool>(v.size()))
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:25: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
+ // CHECK-MESSAGES: :9:8: note: method 'vector'::empty() defined here
+ // CHECK-FIXES: {{^ }}if (static_cast<bool>(!v.empty())){{$}}
+ if (v.size() && false)
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
+ // CHECK-MESSAGES: :9:8: note: method 'vector'::empty() defined here
+ // CHECK-FIXES: {{^ }}if (!v.empty() && false){{$}}
+ if (!v.size())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:8: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
+ // CHECK-MESSAGES: :9:8: note: method 'vector'::empty() defined here
+ // CHECK-FIXES: {{^ }}if (v.empty()){{$}}
+
+ TemplatedContainer<T> templated_container;
+ if (templated_container.size())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here
+ // CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
+ if (templated_container != TemplatedContainer<T>())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here
+ // CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
+ // CHECK-FIXES-NEXT: ;
+ while (templated_container.size())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: the 'empty' method should be used
+ // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here
+ // CHECK-FIXES: {{^ }}while (!templated_container.empty()){{$}}
+
+ do {
+ }
+ while (templated_container.size());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used
+ // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here
+ // CHECK-FIXES: {{^ }}while (!templated_container.empty());
+
+ for (; templated_container.size();)
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: the 'empty' method should be used
+ // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here
+ // CHECK-FIXES: {{^ }}for (; !templated_container.empty();){{$}}
+
+ if (true && templated_container.size())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:15: warning: the 'empty' method should be used
+ // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here
+ // CHECK-FIXES: {{^ }}if (true && !templated_container.empty()){{$}}
+
+ if (true || templated_container.size())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:15: warning: the 'empty' method should be used
+ // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here
+ // CHECK-FIXES: {{^ }}if (true || !templated_container.empty()){{$}}
+
+ if (!templated_container.size())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:8: warning: the 'empty' method should be used
+ // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here
+ // CHECK-FIXES: {{^ }}if (templated_container.empty()){{$}}
+
+ bool b1 = templated_container.size();
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
+ // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here
+ // CHECK-FIXES: {{^ }}bool b1 = !templated_container.empty();
+
+ bool b2(templated_container.size());
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: the 'empty' method should be used
+ // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here
+ // CHECK-FIXES: {{^ }}bool b2(!templated_container.empty());
+
+ auto b3 = static_cast<bool>(templated_container.size());
+ // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: the 'empty' method should be used
+ // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here
+ // CHECK-FIXES: {{^ }}auto b3 = static_cast<bool>(!templated_container.empty());
+
+ auto b4 = (bool)templated_container.size();
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: the 'empty' method should be used
+ // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here
+ // CHECK-FIXES: {{^ }}auto b4 = (bool)!templated_container.empty();
+
+ auto b5 = bool(templated_container.size());
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: the 'empty' method should be used
+ // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here
+ // CHECK-FIXES: {{^ }}auto b5 = bool(!templated_container.empty());
+
+ takesBool(templated_container.size());
+ // We don't detect this one because we don't know the parameter of takesBool
+ // until the type of templated_container.size() is known
+
+ return templated_container.size();
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used
+ // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here
+ // CHECK-FIXES: {{^ }}return !templated_container.empty();
+}
+
+template <typename TypeRequiresSize>
+void instantiatedTemplateWithSizeCall() {
+ TypeRequiresSize t;
+ // The instantiation of the template with std::vector<int> should not
+ // result in changing the template, because we don't know that
+ // TypeRequiresSize generally has `.empty()`
+ if (t.size())
+ ;
+
+ if (t == TypeRequiresSize{})
+ ;
+
+ if (t != TypeRequiresSize{})
+ ;
+}
+
+class TypeWithSize {
+public:
+ TypeWithSize();
+ bool operator==(const TypeWithSize &other) const;
+ bool operator!=(const TypeWithSize &other) const;
+
+ unsigned size() const { return 0; }
+ // Does not have `.empty()`
+};
+
+void instantiator() {
+ instantiatedTemplateWithSizeCall<TypeWithSize>();
+ instantiatedTemplateWithSizeCall<std::vector<int>>();
+}
+
+class ConstructWithBoolField {
+ bool B;
+public:
+ ConstructWithBoolField(const std::vector<int> &C) : B(C.size()) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:57: warning: the 'empty' method should be used
+// CHECK-MESSAGES: :9:8: note: method 'vector'::empty() defined here
+// CHECK-FIXES: {{^ }}ConstructWithBoolField(const std::vector<int> &C) : B(!C.empty()) {}
+};
+
+struct StructWithNSDMI {
+ std::vector<int> C;
+ bool B = C.size();
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: the 'empty' method should be used
+// CHECK-MESSAGES: :9:8: note: method 'vector'::empty() defined here
+// CHECK-FIXES: {{^ }}bool B = !C.empty();
+};
+
+int func(const std::vector<int> &C) {
+ return C.size() ? 0 : 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used
+// CHECK-MESSAGES: :9:8: note: method 'vector'::empty() defined here
+// CHECK-FIXES: {{^ }}return !C.empty() ? 0 : 1;
+}
+
+struct Lazy {
+ constexpr unsigned size() const { return 0; }
+ constexpr bool empty() const { return true; }
+};
+
+constexpr Lazy L;
+static_assert(!L.size(), "");
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: the 'empty' method should be used
+// CHECK-MESSAGES: :663:18: note: method 'Lazy'::empty() defined here
+// CHECK-FIXES: {{^}}static_assert(L.empty(), "");
+
+struct StructWithLazyNoexcept {
+ void func() noexcept(L.size());
+};
Index: clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
@@ -16,6 +16,77 @@
using namespace clang::ast_matchers;
namespace clang {
+namespace ast_matchers {
+AST_POLYMORPHIC_MATCHER_P2(hasAnyArgumentWithParam,
+ AST_POLYMORPHIC_SUPPORTED_TYPES(CallExpr,
+ CXXConstructExpr),
+ internal::Matcher<Expr>, ArgMatcher,
+ internal::Matcher<ParmVarDecl>, ParamMatcher) {
+ BoundNodesTreeBuilder Result;
+ // The first argument of an overloaded member operator is the implicit object
+ // argument of the method which should not be matched against a parameter, so
+ // we skip over it here.
+ BoundNodesTreeBuilder Matches;
+ unsigned ArgIndex = cxxOperatorCallExpr(callee(cxxMethodDecl()))
+ .matches(Node, Finder, &Matches)
+ ? 1
+ : 0;
+ int ParamIndex = 0;
+ for (; ArgIndex < Node.getNumArgs(); ++ArgIndex) {
+ BoundNodesTreeBuilder ArgMatches(*Builder);
+ if (ArgMatcher.matches(*(Node.getArg(ArgIndex)->IgnoreParenCasts()), Finder,
+ &ArgMatches)) {
+ BoundNodesTreeBuilder ParamMatches(ArgMatches);
+ if (expr(anyOf(cxxConstructExpr(hasDeclaration(cxxConstructorDecl(
+ hasParameter(ParamIndex, ParamMatcher)))),
+ callExpr(callee(functionDecl(
+ hasParameter(ParamIndex, ParamMatcher))))))
+ .matches(Node, Finder, &ParamMatches)) {
+ Result.addMatch(ParamMatches);
+ *Builder = std::move(Result);
+ return true;
+ }
+ }
+ ++ParamIndex;
+ }
+ return false;
+}
+
+AST_MATCHER(Expr, usedInBooleanContext) {
+ std::string exprName = "__booleanContextExpr";
+ auto result =
+ expr(expr().bind(exprName),
+ anyOf(hasParent(varDecl(hasType(booleanType()))),
+ hasParent(cxxConstructorDecl(
+ hasAnyConstructorInitializer(cxxCtorInitializer(
+ withInitializer(expr(equalsBoundNode(exprName))),
+ forField(hasType(booleanType())))))),
+ hasParent(fieldDecl(hasType(booleanType()))),
+ hasParent(stmt(anyOf(
+ explicitCastExpr(hasDestinationType(booleanType())),
+ ifStmt(hasCondition(expr(equalsBoundNode(exprName)))),
+ doStmt(hasCondition(expr(equalsBoundNode(exprName)))),
+ whileStmt(hasCondition(expr(equalsBoundNode(exprName)))),
+ forStmt(hasCondition(expr(equalsBoundNode(exprName)))),
+ conditionalOperator(
+ hasCondition(expr(equalsBoundNode(exprName)))),
+ parenListExpr(hasParent(varDecl(hasType(booleanType())))),
+ parenExpr(hasParent(
+ explicitCastExpr(hasDestinationType(booleanType())))),
+ returnStmt(forFunction(returns(booleanType()))),
+ cxxUnresolvedConstructExpr(hasType(booleanType())),
+ callExpr(hasAnyArgumentWithParam(
+ expr(equalsBoundNode(exprName)),
+ parmVarDecl(hasType(booleanType())))),
+ binaryOperator(hasAnyOperatorName("&&", "||")),
+ unaryOperator(hasOperatorName("!")).bind("NegOnSize"))))))
+ .matches(Node, Finder, Builder);
+ Builder->removeBindings([this, exprName](const BoundNodesMap &Nodes) {
+ return Nodes.getNode(exprName).getNodeKind().isNone();
+ });
+ return result;
+}
+} // namespace ast_matchers
namespace tidy {
namespace readability {
@@ -26,18 +97,27 @@
: ClangTidyCheck(Name, Context) {}
void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) {
- const auto ValidContainer = qualType(hasUnqualifiedDesugaredType(
- recordType(hasDeclaration(cxxRecordDecl(isSameOrDerivedFrom(
- namedDecl(
- has(cxxMethodDecl(
- isConst(), parameterCountIs(0), isPublic(),
- hasName("size"),
- returns(qualType(isInteger(), unless(booleanType()))))
- .bind("size")),
- has(cxxMethodDecl(isConst(), parameterCountIs(0), isPublic(),
- hasName("empty"), returns(booleanType()))
- .bind("empty")))
- .bind("container")))))));
+ const auto ValidContainerRecord = cxxRecordDecl(isSameOrDerivedFrom(
+ namedDecl(
+ has(cxxMethodDecl(isConst(), parameterCountIs(0), isPublic(),
+ hasName("size"),
+ returns(qualType(isInteger(), unless(booleanType()),
+ unless(elaboratedType()))))
+ .bind("size")),
+ has(cxxMethodDecl(isConst(), parameterCountIs(0), isPublic(),
+ hasName("empty"), returns(booleanType()))
+ .bind("empty")))
+ .bind("container")));
+
+ const auto ValidContainerNonTemplateType =
+ qualType(hasUnqualifiedDesugaredType(
+ recordType(hasDeclaration(ValidContainerRecord))));
+ const auto ValidContainerTemplateType =
+ qualType(hasUnqualifiedDesugaredType(templateSpecializationType(
+ hasDeclaration(classTemplateDecl(has(ValidContainerRecord))))));
+
+ const auto ValidContainer = qualType(
+ anyOf(ValidContainerNonTemplateType, ValidContainerTemplateType));
const auto WrongUse = traverse(
TK_AsIs,
@@ -52,18 +132,34 @@
anyOf(hasParent(
unaryOperator(hasOperatorName("!")).bind("NegOnSize")),
anything()))),
- hasParent(explicitCastExpr(hasDestinationType(booleanType())))));
+ usedInBooleanContext()));
Finder->addMatcher(
- cxxMemberCallExpr(on(expr(anyOf(hasType(ValidContainer),
+ cxxMemberCallExpr(unless(isInTemplateInstantiation()),
+ on(expr(anyOf(hasType(ValidContainer),
hasType(pointsTo(ValidContainer)),
- hasType(references(ValidContainer))))),
+ hasType(references(ValidContainer))))
+ .bind("MemberCallObject")),
callee(cxxMethodDecl(hasName("size"))), WrongUse,
unless(hasAncestor(cxxMethodDecl(
ofClass(equalsBoundNode("container"))))))
.bind("SizeCallExpr"),
this);
+ Finder->addMatcher(
+ callExpr(has(cxxDependentScopeMemberExpr(
+ hasObjectExpression(
+ expr(anyOf(hasType(ValidContainer),
+ hasType(pointsTo(ValidContainer)),
+ hasType(references(ValidContainer))))
+ .bind("MemberCallObject")),
+ hasMemberName("size"))),
+ WrongUse,
+ unless(hasAncestor(
+ cxxMethodDecl(ofClass(equalsBoundNode("container"))))))
+ .bind("SizeCallExpr"),
+ this);
+
// Empty constructor matcher.
const auto DefaultConstructor = cxxConstructExpr(
hasDeclaration(cxxConstructorDecl(isDefaultConstructor())));
@@ -72,12 +168,11 @@
ignoringImpCasts(stringLiteral(hasSize(0))),
ignoringImpCasts(cxxBindTemporaryExpr(has(DefaultConstructor))),
ignoringImplicit(DefaultConstructor),
- cxxConstructExpr(
- hasDeclaration(cxxConstructorDecl(isCopyConstructor())),
- has(expr(ignoringImpCasts(DefaultConstructor)))),
- cxxConstructExpr(
- hasDeclaration(cxxConstructorDecl(isMoveConstructor())),
- has(expr(ignoringImpCasts(DefaultConstructor)))));
+ cxxConstructExpr(hasDeclaration(cxxConstructorDecl(isCopyConstructor())),
+ has(expr(ignoringImpCasts(DefaultConstructor)))),
+ cxxConstructExpr(hasDeclaration(cxxConstructorDecl(isMoveConstructor())),
+ has(expr(ignoringImpCasts(DefaultConstructor)))),
+ cxxUnresolvedConstructExpr(argumentCountIs(0)));
// Match the object being compared.
const auto STLArg =
anyOf(unaryOperator(
@@ -87,6 +182,7 @@
expr(hasType(ValidContainer)).bind("STLObject"));
Finder->addMatcher(
cxxOperatorCallExpr(
+ unless(isInTemplateInstantiation()),
hasAnyOverloadedOperatorName("==", "!="),
anyOf(allOf(hasArgument(0, WrongComparend), hasArgument(1, STLArg)),
allOf(hasArgument(0, STLArg), hasArgument(1, WrongComparend))),
@@ -94,24 +190,33 @@
cxxMethodDecl(ofClass(equalsBoundNode("container"))))))
.bind("BinCmp"),
this);
+ Finder->addMatcher(
+ binaryOperator(hasAnyOperatorName("==", "!="),
+ anyOf(allOf(hasLHS(WrongComparend), hasRHS(STLArg)),
+ allOf(hasLHS(STLArg), hasRHS(WrongComparend))),
+ unless(hasAncestor(
+ cxxMethodDecl(ofClass(equalsBoundNode("container"))))))
+ .bind("BinCmp"),
+ this);
}
void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) {
- const auto *MemberCall =
- Result.Nodes.getNodeAs<CXXMemberCallExpr>("SizeCallExpr");
+ const auto *MemberCall = Result.Nodes.getNodeAs<Expr>("SizeCallExpr");
+ const auto *MemberCallObject =
+ Result.Nodes.getNodeAs<Expr>("MemberCallObject");
const auto *BinCmp = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("BinCmp");
+ const auto *BinCmpTempl = Result.Nodes.getNodeAs<BinaryOperator>("BinCmp");
const auto *BinaryOp = Result.Nodes.getNodeAs<BinaryOperator>("SizeBinaryOp");
const auto *Pointee = Result.Nodes.getNodeAs<Expr>("Pointee");
const auto *E =
- MemberCall
- ? MemberCall->getImplicitObjectArgument()
+ MemberCallObject
+ ? MemberCallObject
: (Pointee ? Pointee : Result.Nodes.getNodeAs<Expr>("STLObject"));
FixItHint Hint;
std::string ReplacementText = std::string(
Lexer::getSourceText(CharSourceRange::getTokenRange(E->getSourceRange()),
*Result.SourceManager, getLangOpts()));
- if (BinCmp && IsBinaryOrTernary(E)) {
- // Not just a DeclRefExpr, so parenthesize to be on the safe side.
+ if (IsBinaryOrTernary(E) || isa<UnaryOperator>(E)) {
ReplacementText = "(" + ReplacementText + ")";
}
if (E->getType()->isPointerType())
@@ -125,7 +230,13 @@
}
Hint =
FixItHint::CreateReplacement(BinCmp->getSourceRange(), ReplacementText);
- } else if (BinaryOp) { // Determine the correct transformation.
+ } else if (BinCmpTempl) {
+ if (BinCmpTempl->getOpcode() == BinaryOperatorKind::BO_NE) {
+ ReplacementText = "!" + ReplacementText;
+ }
+ Hint = FixItHint::CreateReplacement(BinCmpTempl->getSourceRange(),
+ ReplacementText);
+ } else if (BinaryOp) { // Determine the correct transformation.
bool Negation = false;
const bool ContainerIsLHS =
!llvm::isa<IntegerLiteral>(BinaryOp->getLHS()->IgnoreImpCasts());
@@ -195,15 +306,17 @@
"!" + ReplacementText);
}
- if (MemberCall) {
- diag(MemberCall->getBeginLoc(),
- "the 'empty' method should be used to check "
- "for emptiness instead of 'size'")
+ auto WarnLoc = MemberCall ? MemberCall->getBeginLoc() : SourceLocation{};
+
+ if (WarnLoc.isValid()) {
+ diag(WarnLoc, "the 'empty' method should be used to check "
+ "for emptiness instead of 'size'")
<< Hint;
} else {
- diag(BinCmp->getBeginLoc(),
- "the 'empty' method should be used to check "
- "for emptiness instead of comparing to an empty object")
+ WarnLoc = BinCmpTempl ? BinCmpTempl->getBeginLoc()
+ : (BinCmp ? BinCmp->getBeginLoc() : SourceLocation{});
+ diag(WarnLoc, "the 'empty' method should be used to check "
+ "for emptiness instead of comparing to an empty object")
<< Hint;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
