[PATCH] D78796: [Support] Refactor LEB128 encoding into an input iterator

[Nicolas Guillemot via Phabricator via llvm-commits]

nlguillemot added a comment.

In D78796#2026009 <https://reviews.llvm.org/D78796#2026009>, @dsanders wrote:

> In D78796#2025835 <https://reviews.llvm.org/D78796#2025835>, @nlguillemot wrote:
>
> > In D78796#2025650 <https://reviews.llvm.org/D78796#2025650>, @dsanders wrote:
> >
> > > I'm inclined to agree that the patch series as-is doesn't really warrant the iterators as the interface as no callers have been updated. However, I also don't see much that's iterator specific (ULEBifier would be roughly similar code leaving the iterator portion as trivial wrappers on the ULEBifier) and there are a few places (particularly in tablegen) that are emitting LEB's into containers where the loop to add bytes one by one is just noise and something like `std::copy(to_uleb(...), std::back_inserter(Table));` would be somewhat nice. The loop could easily be hidden in something like `append_uleb(Table, ...)` though I don't think there's a strong argument for (or against) iterators.
> > >
> > > What I would suggest is separating out the byte-sequence generation into a ULEBifier as David suggested but still keeping the iterator object as a thin adapter (effectively implementing that while loop) to support inter-operation with STL functions like std::copy.
> >
> >
> > What is the core issue with the iterator interface that makes it desirable to have something like `ULEBifier<T>` instead?
>
>
> For me it's that iterators reference and indirect into the elements of a container. They shouldn't be the container themselves


If we think of an integer as a container of bits then it's not that different from a normal iterator. In this case we make a copy of the input because it's convenient and makes the interface simpler to use, but we could refactor the code to avoid mutating a copy of the input if that's important.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78796/new/

https://reviews.llvm.org/D78796