[clang-tools-extra] f7b7138 - [clang-tidy] Make `readability-container-data-pointer` more robust
Fabian Wolff via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 18 12:11:19 PST 2022
Author: Fabian Wolff
Date: 2022-01-18T21:08:59+01:00
New Revision: f7b7138a62648f4019c55e4671682af1f851f295
URL: https://github.com/llvm/llvm-project/commit/f7b7138a62648f4019c55e4671682af1f851f295
DIFF: https://github.com/llvm/llvm-project/commit/f7b7138a62648f4019c55e4671682af1f851f295.diff
LOG: [clang-tidy] Make `readability-container-data-pointer` more robust
Fixes PR#52245. I've also added a few test cases beyond PR#52245 that would also fail with the current implementation, which is quite brittle in many respects (e.g. it uses the `hasDescendant()` matcher to find the container that is being accessed, which is very easy to trick, as in the example in PR#52245).
I have not been able to reproduce the second issue mentioned in PR#52245 (namely that using the `data()` member function is suggested even for containers that don't have it), but I've added a test case for it to be sure.
Reviewed By: aaron.ballman
Differential Revision: https://reviews.llvm.org/D113863
Added:
Modified:
clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
index 3a670509ec2e2..d9851a89ebe1b 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
@@ -16,6 +16,13 @@ using namespace clang::ast_matchers;
namespace clang {
namespace tidy {
namespace readability {
+
+constexpr llvm::StringLiteral ContainerExprName = "container-expr";
+constexpr llvm::StringLiteral DerefContainerExprName = "deref-container-expr";
+constexpr llvm::StringLiteral AddrOfContainerExprName =
+ "addr-of-container-expr";
+constexpr llvm::StringLiteral AddressOfName = "address-of";
+
ContainerDataPointerCheck::ContainerDataPointerCheck(StringRef Name,
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context) {}
@@ -38,69 +45,63 @@ void ContainerDataPointerCheck::registerMatchers(MatchFinder *Finder) {
const auto Container =
qualType(anyOf(NonTemplateContainerType, TemplateContainerType));
+ const auto ContainerExpr = anyOf(
+ unaryOperator(
+ hasOperatorName("*"),
+ hasUnaryOperand(
+ expr(hasType(pointsTo(Container))).bind(DerefContainerExprName)))
+ .bind(ContainerExprName),
+ unaryOperator(hasOperatorName("&"),
+ hasUnaryOperand(expr(anyOf(hasType(Container),
+ hasType(references(Container))))
+ .bind(AddrOfContainerExprName)))
+ .bind(ContainerExprName),
+ expr(anyOf(hasType(Container), hasType(pointsTo(Container)),
+ hasType(references(Container))))
+ .bind(ContainerExprName));
+
+ const auto Zero = integerLiteral(equals(0));
+
+ const auto SubscriptOperator = callee(cxxMethodDecl(hasName("operator[]")));
+
Finder->addMatcher(
unaryOperator(
unless(isExpansionInSystemHeader()), hasOperatorName("&"),
- hasUnaryOperand(anyOf(
- ignoringParenImpCasts(
- cxxOperatorCallExpr(
- callee(cxxMethodDecl(hasName("operator[]"))
- .bind("operator[]")),
- argumentCountIs(2),
- hasArgument(
- 0,
- anyOf(ignoringParenImpCasts(
- declRefExpr(
- to(varDecl(anyOf(
- hasType(Container),
- hasType(references(Container))))))
- .bind("var")),
- ignoringParenImpCasts(hasDescendant(
- declRefExpr(
- to(varDecl(anyOf(
- hasType(Container),
- hasType(pointsTo(Container)),
- hasType(references(Container))))))
- .bind("var"))))),
- hasArgument(1,
- ignoringParenImpCasts(
- integerLiteral(equals(0)).bind("zero"))))
- .bind("operator-call")),
- ignoringParenImpCasts(
- cxxMemberCallExpr(
- hasDescendant(
- declRefExpr(to(varDecl(anyOf(
- hasType(Container),
- hasType(references(Container))))))
- .bind("var")),
- argumentCountIs(1),
- hasArgument(0,
- ignoringParenImpCasts(
- integerLiteral(equals(0)).bind("zero"))))
- .bind("member-call")),
- ignoringParenImpCasts(
- arraySubscriptExpr(
- hasLHS(ignoringParenImpCasts(
- declRefExpr(to(varDecl(anyOf(
- hasType(Container),
- hasType(references(Container))))))
- .bind("var"))),
- hasRHS(ignoringParenImpCasts(
- integerLiteral(equals(0)).bind("zero"))))
- .bind("array-subscript")))))
- .bind("address-of"),
+ hasUnaryOperand(expr(
+ anyOf(cxxOperatorCallExpr(SubscriptOperator, argumentCountIs(2),
+ hasArgument(0, ContainerExpr),
+ hasArgument(1, Zero)),
+ cxxMemberCallExpr(SubscriptOperator, on(ContainerExpr),
+ argumentCountIs(1), hasArgument(0, Zero)),
+ arraySubscriptExpr(hasLHS(ContainerExpr), hasRHS(Zero))))))
+ .bind(AddressOfName),
this);
}
void ContainerDataPointerCheck::check(const MatchFinder::MatchResult &Result) {
- const auto *UO = Result.Nodes.getNodeAs<UnaryOperator>("address-of");
- const auto *DRE = Result.Nodes.getNodeAs<DeclRefExpr>("var");
-
- std::string ReplacementText;
- ReplacementText = std::string(Lexer::getSourceText(
- CharSourceRange::getTokenRange(DRE->getSourceRange()),
- *Result.SourceManager, getLangOpts()));
- if (DRE->getType()->isPointerType())
+ const auto *UO = Result.Nodes.getNodeAs<UnaryOperator>(AddressOfName);
+ const auto *CE = Result.Nodes.getNodeAs<Expr>(ContainerExprName);
+ const auto *DCE = Result.Nodes.getNodeAs<Expr>(DerefContainerExprName);
+ const auto *ACE = Result.Nodes.getNodeAs<Expr>(AddrOfContainerExprName);
+
+ if (!UO || !CE)
+ return;
+
+ if (DCE && !CE->getType()->isPointerType())
+ CE = DCE;
+ else if (ACE)
+ CE = ACE;
+
+ SourceRange SrcRange = CE->getSourceRange();
+
+ std::string ReplacementText{
+ Lexer::getSourceText(CharSourceRange::getTokenRange(SrcRange),
+ *Result.SourceManager, getLangOpts())};
+
+ if (!isa<DeclRefExpr, ArraySubscriptExpr, CXXOperatorCallExpr, CallExpr>(CE))
+ ReplacementText = "(" + ReplacementText + ")";
+
+ if (CE->getType()->isPointerType())
ReplacementText += "->data()";
else
ReplacementText += ".data()";
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
index 2c9a75897de23..c3d6aa7cf57b1 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
@@ -109,3 +109,38 @@ void m(const std::vector<T> &v) {
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
// CHECK-FIXES: {{^ }}return v.data();{{$}}
}
+
+template <typename T>
+struct container_without_data {
+ using size_type = size_t;
+ T &operator[](size_type);
+ const T &operator[](size_type) const;
+};
+
+template <typename T>
+const T *n(const container_without_data<T> &c) {
+ // c has no "data" member function, so there should not be a warning here:
+ return &c[0];
+}
+
+const int *o(const std::vector<std::vector<std::vector<int>>> &v, const size_t idx1, const size_t idx2) {
+ return &v[idx1][idx2][0];
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+ // CHECK-FIXES: {{^ }}return v[idx1][idx2].data();{{$}}
+}
+
+std::vector<int> &select(std::vector<int> &u, std::vector<int> &v) {
+ return v;
+}
+
+int *p(std::vector<int> &v1, std::vector<int> &v2) {
+ return &select(*&v1, v2)[0];
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+ // CHECK-FIXES: {{^ }}return select(*&v1, v2).data();{{$}}
+}
+
+int *q(std::vector<int> ***v) {
+ return &(***v)[0];
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
+ // CHECK-FIXES: {{^ }}return (**v)->data();{{$}}
+}
More information about the cfe-commits
mailing list