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

[Lang Hames via Phabricator via llvm-commits]

lhames marked 7 inline comments as done.
lhames added inline comments.


================
Comment at: include/llvm/ADT/fallible_iterator.h:76
+public:
+  using this_fallible_iterator = fallible_iterator<Underlying>;
+
----------------
dblaikie wrote:
> I think you can use "fallible_iterator" without template parameters throughout the class (this would ues the injected class name), probably means you don't need this alias here?
Yep. This is a hangover from when the type name was longer. It can be removed.


================
Comment at: include/llvm/ADT/fallible_iterator.h:100
+  /// Forward const dereference to the underlying iterator.
+  auto operator*() const -> decltype(*std::declval<const Underlying>()) {
+    return *I;
----------------
dblaikie wrote:
> Not sure if the "const Underlying" is correct here.
> 
> Iterator const tends not to be deep, I think? So, for instance a "const std::vector<int>::iterator" is not the same as a "std::vector<int>::const_iterator" - the former cannot be assigned to, but its dereference probably still returns non-const reference. Whereas the latter can be assigned, incremented, etc, but still has a const reference return from op*. Does that make sense?
> 
> All in all, I think you only need the member-const op* (because op* doesn't modify the iterator, only the thing it is iterating over). And it should probably use const in the declval, I think?
You're approaching this from a very different angle to me. I just think of these operators as forwarding: There's a const and a non-const version, and they do whatever the const and non-const version of the underlying iterator do. The wrapper is being as minimalist as possible in the assumptions it places on the Underlying type.


================
Comment at: include/llvm/ADT/fallible_iterator.h:126
+  this_fallible_iterator &operator++() {
+    assert(getErrPtr() && "Can not increment end iterator");
+    if (auto Err = I.inc())
----------------
dblaikie wrote:
> 'Cannot'?
/s/can not/cannot/. :)


================
Comment at: include/llvm/ADT/fallible_iterator.h:211-212
+  void resetCheckedFlag() {
+    (void)!!*getErrPtr();
+    assert(!*getErrPtr() && "Incrementing failure value");
+    *getErrPtr() = Error::success();
----------------
dblaikie wrote:
> Should/could this be replaced with "cantFail"?
I don't think Error's move constructor ever guaranteed that a moved-from error was assignable. It *probably* is, but I'd have to check and formalize this.

Instead here I have forced a check and then written to the error.


================
Comment at: unittests/ADT/FallibleIteratorTest.cpp:22-23
+public:
+  using ObjectValid = enum { ValidObject, InvalidObject };
+  using LinkValid = enum { ValidLink, InvalidLink };
+
----------------
dblaikie wrote:
> I'm a bit surprised to find linker terminology in this test case that's orthogonal to linkers, object files, etc. Do you find they improve readability/framing?
I may be biased, but they definitely do for me. Archive iteration was the original motivating use case for the fallible_iterator pattern, and has the advantage of including both kinds of iterator fail: increment/decrement failures (if the archive header is bad), and dereference failures (if an archive member is malformed). Only the former is tested so far in this test case, but I plan to add a dereference failure test too, to verify the forwarding nature of the dereference operators.


================
Comment at: unittests/ADT/FallibleIteratorTest.cpp:194
+    ++I;
+    (void)!!Err; // Manualy reset the checked flag.
+    ++I;
----------------
dblaikie wrote:
> Might be better as an explicit EXPECT_THAT_ERROR(Succeeded) - if there was a bug that caused this Error to be in a fail state, the test would fail rather indirectly later on inside the fallible iterator implementationg where it does "cantFail' (or the moral equivalent)?
Yep. This is another instance of moved-from-Errors-aren't-assignable/reusable. I could use two errors though.


================
Comment at: unittests/ADT/FallibleIteratorTest.cpp:197
+    EXPECT_THAT_ERROR(std::move(Err), Failed()) << "Expected failure value";
+    cantFail(std::move(Err));
+  }
----------------
dblaikie wrote:
> What's this line for? (it looks to me like the EXPECT_THAT_ERROR on the previous line should have validated the Error state & so there should be no other need to do any Error handling after that?)
Yep -- this is redundant. Good catch. I'll remove it.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57618