<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 10, 2014 at 5:11 AM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="">On Sun, Mar 9, 2014 at 5:51 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>On Sat, Mar 8, 2014 at 8:23 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>> wrote:<br>
> On Sat, Mar 8, 2014 at 12:11 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> Author: aaronballman<br>
>> Date: Sat Mar 8 14:11:24 2014<br>
>> New Revision: 203354<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=203354&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=203354&view=rev</a><br>
>> Log:<br>
>> Adding range-based STL-like helper APIs. llvm::distance() is the range<br>
>> version of std::distance. llvm::copy is the range version of std::copy.<br>
>><br>
>> Modified:<br>
>> llvm/trunk/include/llvm/ADT/iterator_range.h<br>
><br>
><br>
> I don't think these belong in this header at all.<br>
<br>
</div>I was going to put them into STLExtras.h, except that didn't seem<br>
right given that ranges aren't in the STL yet. Do you have a better<br>
suggestion as to where they should live? RangeExtras.h perhaps?<br></blockquote><div><br></div></div><div>I think that if we need this, STLExtras is probably the right place. However, the more I think about it, the less I think these utiltiies are good.</div>
<div class="">
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I'm only adding things which come up several places. I don't mind<br>
using iterators if it's one- or two-off instances, but when it's 4-5+,<br>
it seems beneficial to have range versions of some iterator APIs.<br></blockquote><div><br></div></div><div>After thinking about it more, I really don't agree. See below.</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br>
><br>
>><br>
>> Modified: llvm/trunk/include/llvm/ADT/iterator_range.h<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/iterator_range.h?rev=203354&r1=203353&r2=203354&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/iterator_range.h?rev=203354&r1=203353&r2=203354&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- llvm/trunk/include/llvm/ADT/iterator_range.h (original)<br>
>> +++ llvm/trunk/include/llvm/ADT/iterator_range.h Sat Mar 8 14:11:24 2014<br>
>> @@ -23,6 +23,11 @@<br>
>><br>
>> namespace llvm {<br>
>><br>
>> +template <typename Range><br>
>> +struct range_traits {<br>
>> + typedef typename Range::difference_type difference_type;<br>
>> +};<br>
>> +<br>
><br>
><br>
> There is no documentation for this. We have no pattern or reference from the<br>
> standard about how it should work. I don't even know where you're really<br>
> going here or why we need this at all. It feels like just boilerplate to me.<br>
<br>
</div>I was modelling off the MSVC iterator implementation for how<br>
std::distance was implemented. But I'm fine yanking the traits.<br>
<div><br>
><br>
>> /// \brief A range adaptor for a pair of iterators.<br>
>> ///<br>
>> /// This just wraps two iterators into a range-compatible interface.<br>
>> Nothing<br>
>> @@ -32,6 +37,10 @@ class iterator_range {<br>
>> IteratorT begin_iterator, end_iterator;<br>
>><br>
>> public:<br>
>> + // FIXME: We should be using iterator_traits to determine the<br>
>> + // difference_type, but most of our iterators do not expose anything<br>
>> like it.<br>
>> + typedef int difference_type;<br>
><br>
><br>
> I don't think we should expose this until those iterators are fixed and we<br>
> can use something sensible. "int" doesn't really seem like a sensible choice<br>
> for most of our iterators.<br>
<br>
</div>Probably should be ptrdiff_t, in retrospect. But if the boilerplate is<br>
going away, no need to expose this.<br>
<div><br>
><br>
>><br>
>> +<br>
>> iterator_range() {}<br>
>> iterator_range(IteratorT begin_iterator, IteratorT end_iterator)<br>
>> : begin_iterator(std::move(begin_iterator)),<br>
>> @@ -41,6 +50,19 @@ public:<br>
>> IteratorT end() const { return end_iterator; }<br>
>> };<br>
>><br>
>> +/// \brief Determine the distance between the end() and begin() iterators<br>
>> of<br>
>> +/// a range. Analogous to std::distance().<br>
>> +template <class Range><br>
>> +typename range_traits<Range>::difference_type distance(Range R) {<br>
>> + return std::distance(R.begin(), R.end());<br>
>> +}<br>
><br>
><br>
> Is this really worth providing at this point?<br>
<br>
</div>Yes, it gets used about a half-dozen times at least, and it looks<br>
considerably easier on the eye using the range API.<br></blockquote><div><br></div></div></div><div>The more I think about it, the more strongly I disagree.</div><div><br></div><div>First off, the name makes no sense. There is no "distance" for a range. The word "distance" implies two points, and thus begin and end. I think the code is considerably cleaner getting begin and end and computing the distance between them.</div>
</div></div></div></blockquote><div><br></div><div>This question may be naive, but... If not distance(), can we make use of</div><div> bool empty() const { return begin_iterator == end_iterator; }</div><div>I sometimes see the code like:</div>
<div> if (S.relocation_begin() != S.relocation_end()) { </div><div> ///</div><div> }</div><div>it may be nice to rewrite this as:</div><div> if (!S.relocations().empty()) { ... }</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><div>It also ensures that the intended semantic difference between distance and a size method is conveyed -- if the two points have no direct distance relationship, the computation may be linear. I think that the code with iterators makes this more clear and is not nearly common enough to be an actual burden to type out. Let's go back to just the iterator variant.</div>
<div class="">
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><br>
</div>The iterator form is more boilerplate typing without benefit:<br>
<br>
std::copy(Records->field_begin(), Records->field_end(),<br>
std::back_inserter(Fields));<br>
<br>
vs<br>
<br>
llvm::copy(Records->fields(), std::back_inserter(Fields));<br>
<br>
So I'd disagree that it provides no actual benefit, but would agree<br>
that it's not drastic benefit.<br></blockquote><div> </div></div><div>Here too, I find there to be benefit.</div><div><br></div><div>First, std::copy is ultimately a bit of a hack to deal with output iterators and such. Its uses are very rare. I suspect that most of our uses today could be better written as <foo>.insert(<foo>end(), Records->field_begin(), Records->field_end()); I think this is more clear than std::copy by a large margin. I *do* think that having a range-empowered insert and append would make this simpler.</div>
<div><br></div><div>With std::copy, using a range interface opens really serious semantic questions IMO. Many algorithms are going to (I hope) have more value semantics rather than mutating in-place. Thus they will return results rather than accepting output iterators. But that doesn't make any sense for copy because copy doesn't make any sense in a value-semantic world -- it would just be assignment.</div>
<div><br></div><div>So here too, I think we should just go back to the iterator variant. Trying to define clear and unambiguous APIs to save a *very* small amount of typing (and no complexity really) is just not worth it, especially for APIs as strange and contorted as std::copy.</div>
<div><br></div><div>I strongly suspect that our codebase would be more clear if we completely removed std::copy from it, and instead either used member insert methods or a range based for loop to directly stream into some output-iterator-esque API like an ostream.</div>
</div></div></div>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br><br clear="all"><div><br></div>-- <br><div>Alexey Samsonov, MSK</div>
</div></div>