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

[David Blaikie via Phabricator via llvm-commits]

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

Looks good to me - some optional points on test coverage, etc.



================
Comment at: unittests/ADT/FallibleIteratorTest.cpp:182
+                              {ValidItem, ValidLink},
+                              {InvalidItem, InvalidLink}});
+
----------------
Shouldn't this work without the invalid item? An error check should be required after the loop even if the error is success, right?

(perhaps test both cases? But testing the positive case seems more "interesting" (since it's the lesser requirement, but still a requirement))


================
Comment at: unittests/ADT/FallibleIteratorTest.cpp:282
+
+  auto V1 = *I;
+  EXPECT_THAT_ERROR(V1.takeError(), Succeeded());
----------------
I'd personally consider writing out the type here - it's a bit of a distance from the FallibleDeref definition, and Expected<Item>'s "interesting" enough, I think, though I suppose the Expected's the only bit that's important here)


================
Comment at: unittests/ADT/FallibleIteratorTest.cpp:288-289
+  EXPECT_THAT_ERROR(V2.takeError(), Failed());
+  ++I;
+  EXPECT_THAT_ERROR(std::move(Err), Succeeded());
+  auto V3 = *I;
----------------
Should the Error have any reliable state at this point? (I guess it sort of has to by virtue of "return" being valid - so it can't be in an error state - but it's also not in an "unchecked" state (well, I guess it is because no iterator end comparison has been performed))

Not a sure thing in any case, but I'd probably test for inequality with end here, rather than that the Error is in any particular state.

(wonder if we should sure up the iterator to require this - that an iterator be compared to end before it's incremented (it's in the "unchecked" state and comparison with end is how you "check" it - incrementing could discard the unchecked state, technically speaking)? That might be a bit too stringent though - or perhaps that's already the case & these error comparisons are equivalent to comparison with end, and one or the other is needed?)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57618