[PATCH] D57618: [ADT] Add a fallible_iterator wrapper.

[Lang Hames via Phabricator via llvm-commits]

lhames marked an inline comment as done.
lhames added inline comments.


================
Comment at: include/llvm/ADT/fallible_iterator.h:19
+namespace llvm {
+
+/// A wrapper class for fallible iterators.
----------------
alexshap wrote:
> general comment: @lhames , have you considered using CRTP instead of introducing this wrapper/ forwarding etc ?
I didn't. I approached the design like this:

What does a natural fallible iterator API look like? I think it looks like:

  class MyFallibleIterator {
  public:

    // The usual iterator operations
    MyFallibleIterator(...);
    reference_type operator*();
    const reference_type operator*() const;

    // operator++ / operator-- replaced with inc/dec, which return Error.
    Error inc();
    Error dec();
  };

This makes the implementation of inc/dec easy, and feels (relatively) natural to use in a raw loop where you're doing non-trivial stepping. E.g.:

  auto FI = MyFallibleIterator(...);
  auto FE = MyFallibelIterator(...);

  while (FI != FE) {
    auto A = *FI;
    if (auto Err = FI.inc())
      return Err;
    auto B = *FI;
    doSomethingWithAdjacentValues(A, B);
  }

The (mild) problem with CRTP is that you would have to pass an Error in to the constructor for this 'natural' API, even though it doesn't use it. For that reason I am inclined to stick with the wrapper design as is.


================
Comment at: include/llvm/ADT/fallible_iterator.h:53
+///     Error Err = Error::success();
+///     for (auto &C : A.children()) {
+///       // Loop body only entered when increment succeeds.
----------------
alexshap wrote:
> looks like the argument (Err) is missed here
It is. Thanks!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57618