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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
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;
----------------
lhames wrote:
> 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.
Fair enough - I'd probably be inclined to err on the side of simpler until the functionality was needed, but whatever suits.


================
Comment at: include/llvm/ADT/fallible_iterator.h:211-212
+  void resetCheckedFlag() {
+    (void)!!*getErrPtr();
+    assert(!*getErrPtr() && "Incrementing failure value");
+    *getErrPtr() = Error::success();
----------------
lhames wrote:
> 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.
I'd guess there's probably already a fair bit of code that relies on a moved-from Error being assignable.


================
Comment at: unittests/ADT/FallibleIteratorTest.cpp:22-23
+public:
+  using ObjectValid = enum { ValidObject, InvalidObject };
+  using LinkValid = enum { ValidLink, InvalidLink };
+
----------------
lhames wrote:
> 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.
For me it implies some context I don't have or don't have readily at hand when I read the test - and so I kind of glaze over when I come to this wondering what object/archive context I should have to understand this test, where it doesn't seem like I should need any.

s/Archive/FallibleSequence/
s/ArchiveWalker/FallibleSequenceWalker/

Seems to me like it'd be clearer to a reader coming into this code - well, it'd be clearer to me at least.


================
Comment at: unittests/ADT/FallibleIteratorTest.cpp:28
+    Object(ObjectValid V) : V(V) {}
+    bool isValid() const { return V == ValidObject; }
+
----------------
Having another concept of validity here seems confusing, to me at least - if this needs an arbitrary member function to call to test -> on the iterator, perhaps it could have some other name - so it's not likely to confuse the validity/error checking that's central to this testing?


================
Comment at: unittests/ADT/FallibleIteratorTest.cpp:34
+
+  using Archive = std::vector<std::pair<Object, LinkValid>>;
+
----------------
I'd probably put the "do not iterate this object" bit, inside the Object rather than pairing it up. But up to you. (I understand how this maps to the original context, but it doesn't seem necessary to me here, in a standalone setting)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57618