I see. At any rate, the patch merely simplifies the loop variable for such containers. It does not change the behavior wrt. whether or not a loop is transformed.
On Mon, Jan 12, 2015 at 7:36 PM, David Blaikie <[email protected]> wrote: > > > On Mon, Jan 12, 2015 at 9:44 AM, Daniel Jasper <[email protected]> wrote: > >> Thanks for checking. I'll look into it. >> > > For more context/my thoughts: I think there will be cases we just can't > detect/get right (if there's a function call in the loop and the container > leaks somehow (it's a member variable, etc) we can't know if the user wants > the special behavior or it's just a normal every day loop). I think > clang-modernize has buckets for degree-of-risk for different > transformations & this one may just need to go in a slightly risky bucket. > > I'm not sure if there's a nice way for a user to annotate such a loop to > make it clear to tools that this extra behavior is required. > > >> Do you have specific examples? >> > > greping around I did find at least one, in > llvm/lib/Analysis/IPA/InlineCost.cpp: > > // Note that we *must not* cache the size, this loop grows the worklist. > for (unsigned Idx = 0; Idx != BBWorklist.size(); ++Idx) { > > Finding the second kind (the one that caches size() deliberately so as to > only visit the existing elements while new ones are being added) is harder > to find, since it's more idiomatic in LLVM to cache the size. I know I ran > into at least one such loop in the last few weeks, though. > > - David > > >> >> On Mon, Jan 12, 2015 at 6:41 PM, David Blaikie <[email protected]> >> wrote: >> >>> >>> >>> On Mon, Jan 12, 2015 at 5:17 AM, Daniel Jasper <[email protected]> >>> wrote: >>> >>>> Author: djasper >>>> Date: Mon Jan 12 07:17:56 2015 >>>> New Revision: 225629 >>>> >>>> URL: http://llvm.org/viewvc/llvm-project?rev=225629&view=rev >>>> Log: >>>> Make LoopConvert work with containers that are used like arrays. >>>> >>> >>> Are these transformations classified as 'risky' in some way? These sort >>> of loops appear in a few places in LLVM deliberately because we're adding >>> elements to the sequence as we're iterating - so we check size each time >>> through and we access the vector with [] because its buffer might've been >>> invalidated by new insertions. (even with size() accessed once and cached, >>> we sometimes do this to visit pre-existing elements and ignore newly added >>> elements - which would still be broken by a transformation to >>> range-based-for/iterators) >>> >>> >>>> >>>> Modified: >>>> clang-tools-extra/trunk/clang-modernize/LoopConvert/LoopActions.cpp >>>> >>>> clang-tools-extra/trunk/test/clang-modernize/LoopConvert/naming-alias.cpp >>>> >>>> Modified: >>>> clang-tools-extra/trunk/clang-modernize/LoopConvert/LoopActions.cpp >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-modernize/LoopConvert/LoopActions.cpp?rev=225629&r1=225628&r2=225629&view=diff >>>> >>>> ============================================================================== >>>> --- clang-tools-extra/trunk/clang-modernize/LoopConvert/LoopActions.cpp >>>> (original) >>>> +++ clang-tools-extra/trunk/clang-modernize/LoopConvert/LoopActions.cpp >>>> Mon Jan 12 07:17:56 2015 >>>> @@ -450,9 +450,16 @@ static bool isAliasDecl(const Decl *TheD >>>> const CXXOperatorCallExpr *OpCall = >>>> cast<CXXOperatorCallExpr>(Init); >>>> if (OpCall->getOperator() == OO_Star) >>>> return isDereferenceOfOpCall(OpCall, IndexVar); >>>> + if (OpCall->getOperator() == OO_Subscript) { >>>> + assert(OpCall->getNumArgs() == 2); >>>> + return true; >>>> + } >>>> break; >>>> } >>>> >>>> + case Stmt::CXXMemberCallExprClass: >>>> + return true; >>>> + >>>> default: >>>> break; >>>> } >>>> >>>> Modified: >>>> clang-tools-extra/trunk/test/clang-modernize/LoopConvert/naming-alias.cpp >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-modernize/LoopConvert/naming-alias.cpp?rev=225629&r1=225628&r2=225629&view=diff >>>> >>>> ============================================================================== >>>> --- >>>> clang-tools-extra/trunk/test/clang-modernize/LoopConvert/naming-alias.cpp >>>> (original) >>>> +++ >>>> clang-tools-extra/trunk/test/clang-modernize/LoopConvert/naming-alias.cpp >>>> Mon Jan 12 07:17:56 2015 >>>> @@ -7,6 +7,8 @@ >>>> const int N = 10; >>>> >>>> Val Arr[N]; >>>> +dependent<Val> v; >>>> +dependent<Val> *pv; >>>> Val &func(Val &); >>>> void sideEffect(int); >>>> >>>> @@ -50,6 +52,25 @@ void aliasing() { >>>> // CHECK-NEXT: int y = t.x; >>>> // CHECK-NEXT: int z = elem.x + t.x; >>>> >>>> + // The same for pseudo-arrays like std::vector<T> (or here >>>> dependent<Val>) >>>> + // which provide a subscript operator[]. >>>> + for (int i = 0; i < v.size(); ++i) { >>>> + Val &t = v[i]; { } >>>> + int y = t.x; >>>> + } >>>> + // CHECK: for (auto & t : v) >>>> + // CHECK-NEXT: { } >>>> + // CHECK-NEXT: int y = t.x; >>>> + >>>> + // The same with a call to at() >>>> + for (int i = 0; i < pv->size(); ++i) { >>>> + Val &t = pv->at(i); { } >>>> + int y = t.x; >>>> + } >>>> + // CHECK: for (auto & t : *pv) >>>> + // CHECK-NEXT: { } >>>> + // CHECK-NEXT: int y = t.x; >>>> + >>>> for (int i = 0; i < N; ++i) { >>>> Val &t = func(Arr[i]); >>>> int y = t.x; >>>> >>>> >>>> _______________________________________________ >>>> cfe-commits mailing list >>>> [email protected] >>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>> >>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
