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

[Michael Kitzan via Phabricator via llvm-commits]

mkitzan added a comment.

Hey Nicolas, just add some thoughts on this patch.



================
Comment at: llvm/include/llvm/Support/LEB128.h:50
+    CurrByte = Value & 0x7f;
+    if (IsSigned) {
+      int64_t SValue = static_cast<int64_t>(Value);
----------------
dsanders wrote:
> Is testing this on each byte measurably slower? It seems unlikely but this is called a lot and I notice that the previous code didn't do it. If it does, it would be good if we can have the template instantiation pick one side or the other
`IsSigned` could easily be changed to a template parameter. Could then turn this is into `if constexpr (IsSigned)` (if you can use C++17), or just SFINAE it. Better yet, use [[ https://en.cppreference.com/w/cpp/types/is_signed | `is_signed` type trait ]] on `ValueT`.


================
Comment at: llvm/include/llvm/Support/LEB128.h:80
+  /// Constructs the end-of-input iterator.
+  LEB128InputIterator() : IsEnd(true) {}
+
----------------
This constructor leaves other data members uninitialized which may be fine for an end iterator, but is a code smell [[ http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-complete | C.41 ]].


================
Comment at: llvm/include/llvm/Support/LEB128.h:93-95
+  explicit LEB128InputIterator(ValueT Value, bool IsSigned, unsigned PadTo)
+      : IsEnd(false), Value(std::move(Value)), IsSigned(IsSigned), PadTo(PadTo),
+        Count(0) {
----------------
Would be a good idea to initialize `More` in the initializer list just in case something changes in the future and `More` is read before being initially assigned to in `encodeNextByte`. Could also apply to `CurrByte`...


================
Comment at: llvm/include/llvm/Support/LEB128.h:132
+      // Add a continuation bit to signal that there is more padding after this.
+      if (Count < PadTo - 1)
+        CurrByte |= 0x80;
----------------
If `PadTo` is 0, may have unintentional underflow since it is `unsigned` (this may be you intention though, and if so it's worth a comment).

Seeing it a lot in the other code. Maybe underflow is part of the plan here?


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

https://reviews.llvm.org/D78796