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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders accepted this revision.
dsanders added a comment.
This revision is now accepted and ready to land.

LGTM

FWIW, I still feel the encoder state shouldn't live inside the iterator but I am a bit happier that the iterator doesn't own the non-trivial encoder logic anymore. It's more consistent with pointing into a container rather than being the container, even if the pointer is unusually descriptive and the container is really more of a description of all the possible containers disambiguated by references to a specific element. I do wonder why do the logic and not the storage though. All that said, pragmatically I don't really want to insist on that as it's been in review for a very long time now so I think the middle ground is, LGTM as it currently is and if someone needs to use it in a generator-like style at some point they'll be the ones to put detail::encodeLEB128Byte() and the state in an object and have the iterators use that. Similarly, if we grow more places where iterators don't make sense they can either use the existing functions or do the same transformation just described.


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

https://reviews.llvm.org/D78796