[PATCH] D15068: ScopInfo: Replace while/iterator construct with std::remove_if

[Tobias Grosser via llvm-commits]

On 11/30/2015 04:49 PM, David Blaikie wrote:
>
>
> On Mon, Nov 30, 2015 at 1:51 AM, Michael Kruse <llvm at meinersbur.de
> <mailto:llvm at meinersbur.de>> wrote:
>
>     2015-11-30 6:53 GMT+01:00 David Blaikie <dblaikie at gmail.com
>     <mailto:dblaikie at gmail.com>>:
>      >
>      >
>      > On Sun, Nov 29, 2015 at 9:36 PM, Tobias Grosser via llvm-commits
>      > <llvm-commits at lists.llvm.org
>     <mailto:llvm-commits at lists.llvm.org>> wrote:
>      >>
>      >> grosser created this revision.
>      >> grosser added reviewers: jdoerfert, Meinersbur.
>      >> grosser added subscribers: llvm-commits, pollydev.
>      >>
>      >> The use of C++'s high-level iterator functionality instead of
>     two while
>      >> loops
>      >> and explicit iterator handling improves readability of this code.
>      >>
>      >> This passes LNT -polly-process-unprofitable for me.
>      >>
>      >> Proposed-by: Michael Kruse <llvm at meinersbur.de
>     <mailto:llvm at meinersbur.de>>
>      >>
>      >> http://reviews.llvm.org/D15068
>      >>
>      >> Files:
>      >>   lib/Analysis/ScopInfo.cpp
>      >>
>      >> Index: lib/Analysis/ScopInfo.cpp
>      >> ===================================================================
>      >> --- lib/Analysis/ScopInfo.cpp
>      >> +++ lib/Analysis/ScopInfo.cpp
>      >> @@ -1464,28 +1464,18 @@
>      >>  void ScopStmt::dump() const { print(dbgs()); }
>      >>
>      >>  void ScopStmt::removeMemoryAccesses(MemoryAccessList &InvMAs) {
>      >> -
>      >> -  // Remove all memory accesses in @p InvMAs from this
>     statement together
>      >> -  // with all scalar accesses that were caused by them. The tricky
>      >> iteration
>      >> -  // order uses is needed because the MemAccs is a vector and
>     the order
>      >> in
>      >> -  // which the accesses of each memory access list (MAL) are
>     stored in
>      >> this
>      >> -  // vector is reversed.
>      >> +  // Remove all memory accesses in @p InvMAs from this statement
>      >> +  // together with all scalar accesses that were caused by them.
>      >>    for (MemoryAccess *MA : InvMAs) {
>      >> -    auto &MAL = *lookupAccessesFor(MA->getAccessInstruction());
>      >> -    MAL.reverse();
>      >> -
>      >> -    auto MALIt = MAL.begin();
>      >> -    auto MALEnd = MAL.end();
>      >> -    auto MemAccsIt = MemAccs.begin();
>      >> -    while (MALIt != MALEnd) {
>      >> -      while (*MemAccsIt != *MALIt)
>      >> -        MemAccsIt++;
>      >> -
>      >> -      MALIt++;
>      >> -      MemAccs.erase(MemAccsIt);
>      >> -    }
>      >> -
>      >> +    auto Predicate = [MA](MemoryAccess *Acc) -> bool {
>      >
>      >
>      > Just capture everything by ref here ^ ("[&]") since the lambda is
>     being used
>      > in the scope (it's not leaking out via std::function, etc) so
>     everything
>      > will be valid & it should just be treated like a normal scope.
>
>     Is this the preferred style or is there some other reason?
>
>
> 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).
>
> 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)
>
>
>
>     >>
>     >> +             (Acc->isWrite() &&
>     >> +              Acc->getAccessInstruction() == MA->getAccessInstruction());
>     >> +    };
>     >> +    MemAccs.erase(std::remove_if(MemAccs.begin(), MemAccs.end(),
>     >> Predicate),
>     >
>     >
>     > Probably just define the lambda directly inline without giving it a name?
>
>      From where Tobias extracted the code from
>     (http://reviews.llvm.org/D13676), the lambda was used twice. D13676 is
>     still under review.
>
>
> ah, thanks for the context

Thank you all for your review comments. I adopted all proposed changes 
(besides removing the Predicate variable) and committed the change in 
254305.

Best,
Tobias