fwolff updated this revision to Diff 391869. fwolff marked an inline comment as done.
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,12 @@ 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 +44,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