fwolff updated this revision to Diff 391871.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113863/new/
https://reviews.llvm.org/D113863
Files:
clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
@@ -109,3 +109,38 @@
// 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();{{$}}
+}
Index: clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
@@ -16,6 +16,13 @@
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,65 @@
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");
+ 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;
+
+ const auto SourceRange = CE->getSourceRange();
std::string ReplacementText;
- ReplacementText = std::string(Lexer::getSourceText(
- CharSourceRange::getTokenRange(DRE->getSourceRange()),
- *Result.SourceManager, getLangOpts()));
- if (DRE->getType()->isPointerType())
+ ReplacementText = std::string(
+ Lexer::getSourceText(CharSourceRange::getTokenRange(SourceRange),
+ *Result.SourceManager, getLangOpts()));
+
+ if (!isa<DeclRefExpr>(CE) && !isa<ArraySubscriptExpr>(CE) &&
+ !isa<CXXOperatorCallExpr>(CE) && !isa<CallExpr>(CE))
+ ReplacementText = "(" + ReplacementText + ")";
+
+ if (CE->getType()->isPointerType())
ReplacementText += "->data()";
else
ReplacementText += ".data()";
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
