Author: poelmanc Date: 2021-02-07T16:36:34Z New Revision: 5229edd66742aeed407f774d77d437fa80347ad1
URL: https://github.com/llvm/llvm-project/commit/5229edd66742aeed407f774d77d437fa80347ad1 DIFF: https://github.com/llvm/llvm-project/commit/5229edd66742aeed407f774d77d437fa80347ad1.diff LOG: [clang-tidy] fix modernize-loop-convert to retain needed array-like operator[] `modernize-loop-convert` handles //array-like// objects like vectors fairly well, but strips slightly too much information from the iteration expression by converting: ``` Vector<Vector<int>> X; for (int J = 0; J < X[5].size(); ++J) copyArg(X[5][J]); ``` to ``` Vector<Vector<int>> X; for (int J : X) // should be for (int J : X[5]) copyArg(J); ``` The `[5]` is a call to `operator[]` and gets stripped by `LoopConvertCheck::getContainerString`. This patch fixes that and adds several test cases. Reviewed By: njames93 Differential Revision: https://reviews.llvm.org/D95771 Added: clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-multidimensional.cpp Modified: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp index e6b7c1f6025e..ec75be9c5f53 100644 --- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp @@ -712,10 +712,11 @@ StringRef LoopConvertCheck::getContainerString(ASTContext *Context, if (isa<CXXThisExpr>(ContainerExpr)) { ContainerString = "this"; } else { - // For CXXOperatorCallExpr (e.g. vector_ptr->size()), its first argument is - // the class object (vector_ptr) we are targeting. + // For CXXOperatorCallExpr such as vector_ptr->size() we want the class + // object vector_ptr, but for vector[2] we need the whole expression. if (const auto* E = dyn_cast<CXXOperatorCallExpr>(ContainerExpr)) - ContainerExpr = E->getArg(0); + if (E->getOperator() != OO_Subscript) + ContainerExpr = E->getArg(0); ContainerString = getStringFromRange(Context->getSourceManager(), Context->getLangOpts(), ContainerExpr->getSourceRange()); diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-multidimensional.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-multidimensional.cpp new file mode 100644 index 000000000000..bffb7f560559 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-multidimensional.cpp @@ -0,0 +1,79 @@ +// RUN: %check_clang_tidy %s modernize-loop-convert %t + +template <class T> +struct Vector { + using iterator = T*; + unsigned size() const; + const T &operator[](int) const; + T &operator[](int); + T *begin(); + T *end(); + const T *begin() const; + const T *end() const; +}; + +template <typename T> +void copyArg(T); + +class TestArrayOfVector { + Vector<int> W[2]; + + void foo() const { + for (int I = 0; I < W[0].size(); ++I) { + if (W[0][I]) + copyArg(W[0][I]); + } + // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: use range-based for loop + // CHECK-FIXES: for (int I : W[0]) { + // CHECK-FIXES-NEXT: if (I) + // CHECK-FIXES-NEXT: copyArg(I); + } +}; + +class TestVectorOfVector { + Vector<Vector<int>> X; + + void foo() const { + for (int J = 0; J < X[0].size(); ++J) { + if (X[0][J]) + copyArg(X[0][J]); + } + // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: use range-based for loop + // CHECK-FIXES: for (int J : X[0]) { + // CHECK-FIXES-NEXT: if (J) + // CHECK-FIXES-NEXT: copyArg(J); + } +}; + +void testVectorOfVectorOfVector() { + Vector<Vector<Vector<int>>> Y; + for (int J = 0; J < Y[3].size(); ++J) { + if (Y[3][J][7]) + copyArg(Y[3][J][8]); + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop + // CHECK-FIXES: for (auto & J : Y[3]) { + // CHECK-FIXES-NEXT: if (J[7]) + // CHECK-FIXES-NEXT: copyArg(J[8]); + + for (int J = 0; J < Y[3][4].size(); ++J) { + if (Y[3][4][J]) + copyArg(Y[3][4][J]); + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop + // CHECK-FIXES: for (int J : Y[3][4]) { + // CHECK-FIXES-NEXT: if (J) + // CHECK-FIXES-NEXT: copyArg(J); +} + +void testVectorOfVectorIterator() { + Vector<Vector<int>> Z; + for (Vector<int>::iterator it = Z[4].begin(); it != Z[4].end(); ++it) { + if (*it) + copyArg(*it); + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop + // CHECK-FIXES: for (int & it : Z[4]) { + // CHECK-FIXES-NEXT: if (it) + // CHECK-FIXES-NEXT: copyArg(it); +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
