On Tue, Jan 13, 2015 at 6:59 AM, Daniel Jasper <[email protected]> wrote:
> 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. > Ah, fair enough. What did the transformation look like before your change? > > 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
