<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Nov 30, 2015 at 1:51 AM, Michael Kruse <span dir="ltr"><<a href="mailto:llvm@meinersbur.de" target="_blank">llvm@meinersbur.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">2015-11-30 6:53 GMT+01:00 David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>>:<br>
><br>
><br>
> On Sun, Nov 29, 2015 at 9:36 PM, Tobias Grosser via llvm-commits<br>
> <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> grosser created this revision.<br>
>> grosser added reviewers: jdoerfert, Meinersbur.<br>
>> grosser added subscribers: llvm-commits, pollydev.<br>
>><br>
>> The use of C++'s high-level iterator functionality instead of two while<br>
>> loops<br>
>> and explicit iterator handling improves readability of this code.<br>
>><br>
>> This passes LNT -polly-process-unprofitable for me.<br>
>><br>
>> Proposed-by: Michael Kruse <<a href="mailto:llvm@meinersbur.de">llvm@meinersbur.de</a>><br>
>><br>
>> <a href="http://reviews.llvm.org/D15068" rel="noreferrer" target="_blank">http://reviews.llvm.org/D15068</a><br>
>><br>
>> Files:<br>
>> lib/Analysis/ScopInfo.cpp<br>
>><br>
>> Index: lib/Analysis/ScopInfo.cpp<br>
>> ===================================================================<br>
>> --- lib/Analysis/ScopInfo.cpp<br>
>> +++ lib/Analysis/ScopInfo.cpp<br>
>> @@ -1464,28 +1464,18 @@<br>
>> void ScopStmt::dump() const { print(dbgs()); }<br>
>><br>
>> void ScopStmt::removeMemoryAccesses(MemoryAccessList &InvMAs) {<br>
>> -<br>
>> - // Remove all memory accesses in @p InvMAs from this statement together<br>
>> - // with all scalar accesses that were caused by them. The tricky<br>
>> iteration<br>
>> - // order uses is needed because the MemAccs is a vector and the order<br>
>> in<br>
>> - // which the accesses of each memory access list (MAL) are stored in<br>
>> this<br>
>> - // vector is reversed.<br>
>> + // Remove all memory accesses in @p InvMAs from this statement<br>
>> + // together with all scalar accesses that were caused by them.<br>
>> for (MemoryAccess *MA : InvMAs) {<br>
>> - auto &MAL = *lookupAccessesFor(MA->getAccessInstruction());<br>
>> - MAL.reverse();<br>
>> -<br>
>> - auto MALIt = MAL.begin();<br>
>> - auto MALEnd = MAL.end();<br>
>> - auto MemAccsIt = MemAccs.begin();<br>
>> - while (MALIt != MALEnd) {<br>
>> - while (*MemAccsIt != *MALIt)<br>
>> - MemAccsIt++;<br>
>> -<br>
>> - MALIt++;<br>
>> - MemAccs.erase(MemAccsIt);<br>
>> - }<br>
>> -<br>
>> + auto Predicate = [MA](MemoryAccess *Acc) -> bool {<br>
><br>
><br>
> Just capture everything by ref here ^ ("[&]") since the lambda is being used<br>
> in the scope (it's not leaking out via std::function, etc) so everything<br>
> will be valid & it should just be treated like a normal scope.<br>
<br>
</div></div>Is this the preferred style or is there some other reason?<br></blockquote><div><br></div><div>Just stylistic - open to discussion (the specific ins and outs of C++11 style both across the field and across LLVM's codebase are still in flux, for sure).<br><br>I've mentioned this on one or two other threads, but my feeling is that if the lambda is only used within the lexical scope (& especially if it's only used within a single full expression as with most lambda usage in standard and standard-esque algorithms) then it should work like another nested lexical scope: simply capture everything implicitly by reference. Anything else and it's likely to be a surprise (especially a capture by value default, but ever just explicit captures might cause undue friction/maintenance pain)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
<br>
>><br>
>> + (Acc->isWrite() &&<br>
>> + Acc->getAccessInstruction() == MA->getAccessInstruction());<br>
>> + };<br>
>> + MemAccs.erase(std::remove_if(MemAccs.begin(), MemAccs.end(),<br>
>> Predicate),<br>
><br>
><br>
> Probably just define the lambda directly inline without giving it a name?<br>
<br>
</span>From where Tobias extracted the code from<br>
(<a href="http://reviews.llvm.org/D13676" rel="noreferrer" target="_blank">http://reviews.llvm.org/D13676</a>), the lambda was used twice. D13676 is<br>
still under review.<br></blockquote><div><br></div><div>ah, thanks for the context</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
Michael<br>
</font></span></blockquote></div><br></div></div>