[PATCH] D36089: [ELF] - Replace parallelForEach with ranged form.

[David Blaikie via llvm-commits]

On Tue, Aug 1, 2017 at 6:41 PM Rafael Avila de Espindola <
rafael.espindola at gmail.com> wrote:

> Pete Cooper <peter_cooper at apple.com> writes:
>
> >> On Aug 1, 2017, at 4:16 PM, Rafael Avila de Espindola <
> rafael.espindola at gmail.com> wrote:
> >>
> >> George Rimar <grimar at accesssoftek.com> writes:
> >>
> >>> I used LLVM algorithms like llvm::find_if, llvm::find as areference.
> >>> They use r-value and I think they do that to support constructions
> like:
> >>> parallelForEach(std::vector<int>({1, 2, 3}), [](int) { ... });
> >>>
> >>> I could probably use "const R &Range" for supporting above,
> >>> but use of r-value is just consistent.
> >>
> >> Pete, looks like you added the first one of these (all_of). Do you
> >> remember why you used r-value instead of const references?
> > Looks like it spun out of feedback from Dave Blaikie, but I don't see a
> specific suggestion to use r-value, so i honestly can't really remember.
> >
> > See
> http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20150511/276117.html
> <
> http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20150511/276117.html>
> for more details, not that it has the r-value stuff.
>

Sure that's the right link, Pete? Not quite connecting the dots/relevance
here.


>
> David,
>
> Do you remember why all_of and related functions use r-value references
> instead of const references?
>

This becomes more relevant with range adapters rather than range algorithms
- the trick being that you want to take ownership of the range if it's an
rvalue (note that T&& isn't an rvalue necessarily - it's a universal
reference... or someth other terminology. It can refer to an rvalue or an
lvalue - I can describe all that in more detail if you're not familiar with
that weird/complex part of C++) otherwise range-for loops like this:

for (auto x : reverse(buildVector()))

break - if buildVector returns a vector by value, and 'reverse' produces a
range adapter that refers to the vector but doesn't take ownership - then
the vector will be destroyed after the evaluation of the range-for loops
initializer expression, leaving the reverse adapting range with a dangling
reference to the underlying vector. (but on the other hand, when presented
with a non-rvalue reference, you don't want to unduly copy the contents...
- so range adapters do dark magic things to correctly copy or not as
needed... at least range adapters attempting to be safe/highly generic. I
think some of the range adapters so far in LLVM have these sort of smarts)

I suppose for a range algorithm (as opposed to a range adapter) you
probably just need to take by T & (and T will be deduced to const U if
needed).

Taking by T&& and not using std::forward is pretty telling that it's
probably not needed/intended. And using std::forward more than once, like
using std::move more than once, is a bit suspect (though I've done it in
some specific cases).

So if this was a range-algorithm calling through to another range
algorithm, I might use T&& and std::forward - to pass through the
mutability/const/rvalueness, etc... - but that's not the case here.

Unless there are counterexamples already in ADT - happy to speculate on
their design and the consistency/inconsistency with this code.


>
> Cheers,
> Rafael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170802/823d7535/attachment.html>