steveire created this revision.
steveire added a reviewer: alexfh.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
steveire requested review of this revision.
readability-container-size-empty currently modifies source code based on
AST nodes in template instantiations, which means that it makes
transformations based on substituted types. This can lead to
transforming code to be broken.
Change the matcher implementation to ignore template instantiations
explicitly, and add a matcher to explicitly handle template declatations
instead of instantiations.
Repository:
rG LLVM Github Monorepo
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
@@ -442,3 +442,83 @@
f<double>();
f<char *>();
}
+
+template <typename T>
+void 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: ;
+}
+
+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>>();
+}
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
@@ -26,18 +26,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(
ast_type_traits::TK_AsIs,
@@ -52,18 +61,37 @@
anyOf(hasParent(
unaryOperator(hasOperatorName("!")).bind("NegOnSize")),
anything()))),
- hasParent(explicitCastExpr(hasDestinationType(booleanType())))));
+ hasParent(explicitCastExpr(hasDestinationType(booleanType()))),
+ hasParent(ifStmt()), hasParent(whileStmt()),
+ hasParent(binaryOperator(hasAnyOperatorName("&&", "||"))),
+ hasParent(unaryOperator(hasOperatorName("!")).bind("NegOnSize"))));
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 +100,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(argummentCountIs(0)));
// Match the object being compared.
const auto STLArg =
anyOf(unaryOperator(
@@ -87,6 +114,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 +122,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 +162,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 +238,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
