<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jan 12, 2015 at 9:44 AM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Thanks for checking. I'll look into it.</div></blockquote><div><br>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.<br><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"> Do you have specific examples?</div></blockquote><div><br>greping around I did find at least one, in llvm/lib/Analysis/IPA/InlineCost.cpp:<br><br><div> // Note that we *must not* cache the size, this loop grows the worklist.</div><div> for (unsigned Idx = 0; Idx != BBWorklist.size(); ++Idx) {<br><br>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.<br><br>- David</div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jan 12, 2015 at 6:41 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Mon, Jan 12, 2015 at 5:17 AM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: djasper<br>
Date: Mon Jan 12 07:17:56 2015<br>
New Revision: 225629<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=225629&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=225629&view=rev</a><br>
Log:<br>
Make LoopConvert work with containers that are used like arrays.<br></blockquote></span><div><br>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)<br> </div><div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Modified:<br>
clang-tools-extra/trunk/clang-modernize/LoopConvert/LoopActions.cpp<br>
clang-tools-extra/trunk/test/clang-modernize/LoopConvert/naming-alias.cpp<br>
<br>
Modified: clang-tools-extra/trunk/clang-modernize/LoopConvert/LoopActions.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-modernize/LoopConvert/LoopActions.cpp?rev=225629&r1=225628&r2=225629&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-modernize/LoopConvert/LoopActions.cpp?rev=225629&r1=225628&r2=225629&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clang-modernize/LoopConvert/LoopActions.cpp (original)<br>
+++ clang-tools-extra/trunk/clang-modernize/LoopConvert/LoopActions.cpp Mon Jan 12 07:17:56 2015<br>
@@ -450,9 +450,16 @@ static bool isAliasDecl(const Decl *TheD<br>
const CXXOperatorCallExpr *OpCall = cast<CXXOperatorCallExpr>(Init);<br>
if (OpCall->getOperator() == OO_Star)<br>
return isDereferenceOfOpCall(OpCall, IndexVar);<br>
+ if (OpCall->getOperator() == OO_Subscript) {<br>
+ assert(OpCall->getNumArgs() == 2);<br>
+ return true;<br>
+ }<br>
break;<br>
}<br>
<br>
+ case Stmt::CXXMemberCallExprClass:<br>
+ return true;<br>
+<br>
default:<br>
break;<br>
}<br>
<br>
Modified: clang-tools-extra/trunk/test/clang-modernize/LoopConvert/naming-alias.cpp<br>
URL: <a href="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" target="_blank">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</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/test/clang-modernize/LoopConvert/naming-alias.cpp (original)<br>
+++ clang-tools-extra/trunk/test/clang-modernize/LoopConvert/naming-alias.cpp Mon Jan 12 07:17:56 2015<br>
@@ -7,6 +7,8 @@<br>
const int N = 10;<br>
<br>
Val Arr[N];<br>
+dependent<Val> v;<br>
+dependent<Val> *pv;<br>
Val &func(Val &);<br>
void sideEffect(int);<br>
<br>
@@ -50,6 +52,25 @@ void aliasing() {<br>
// CHECK-NEXT: int y = t.x;<br>
// CHECK-NEXT: int z = elem.x + t.x;<br>
<br>
+ // The same for pseudo-arrays like std::vector<T> (or here dependent<Val>)<br>
+ // which provide a subscript operator[].<br>
+ for (int i = 0; i < v.size(); ++i) {<br>
+ Val &t = v[i]; { }<br>
+ int y = t.x;<br>
+ }<br>
+ // CHECK: for (auto & t : v)<br>
+ // CHECK-NEXT: { }<br>
+ // CHECK-NEXT: int y = t.x;<br>
+<br>
+ // The same with a call to at()<br>
+ for (int i = 0; i < pv->size(); ++i) {<br>
+ Val &t = pv->at(i); { }<br>
+ int y = t.x;<br>
+ }<br>
+ // CHECK: for (auto & t : *pv)<br>
+ // CHECK-NEXT: { }<br>
+ // CHECK-NEXT: int y = t.x;<br>
+<br>
for (int i = 0; i < N; ++i) {<br>
Val &t = func(Arr[i]);<br>
int y = t.x;<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>