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

[Lang Hames via Phabricator via llvm-commits]

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


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:176
   }
   if (Err)
     return createFileError(Config.InputFilename, std::move(Err));
----------------
rupprecht wrote:
> When I remove this Error check at trunk, I can get 6 tests to fail:
> 
> ```
> $ ninja check-llvm-tools-llvm-objcopy
> ...
> Program aborted due to an unhandled Error:
> Error value was Success. (Note: Success values must still be checked prior to being destroyed).
> ...
> Failing Tests (6):
>     LLVM :: tools/llvm-objcopy/ELF/basic-archive-copy.test
>     LLVM :: tools/llvm-objcopy/ELF/deterministic-archive.test
>     LLVM :: tools/llvm-objcopy/ELF/strip-all.test
>     LLVM :: tools/llvm-objcopy/ELF/strip-debug.test
>     LLVM :: tools/llvm-objcopy/ELF/strip-preserve-atime.test
>     LLVM :: tools/llvm-objcopy/ELF/strip-preserve-mtime.test
> ```
> 
> But when I patch in this change and then remove this line, I no longer get an error that Err is unchecked. So, I think something needs to reset Error so that it is unchecked at the end of normal loop iteration.
I was not able to reproduce that. Can you try again with the latest patch? Is this happening on a debug or release branch?


================
Comment at: unittests/ADT/FallibleIteratorTest.cpp:182
+                              {ValidItem, ValidLink},
+                              {InvalidItem, InvalidLink}});
+
----------------
dblaikie wrote:
> 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))
I have removed that item: the loop only ranges over the first two.


================
Comment at: unittests/ADT/FallibleIteratorTest.cpp:282
+
+  auto V1 = *I;
+  EXPECT_THAT_ERROR(V1.takeError(), Succeeded());
----------------
dblaikie wrote:
> 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)
Done.


================
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;
----------------
dblaikie wrote:
> 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?)
> wonder if we should sure up the iterator to require this - that an iterator be compared to end before it's incremented

Good call. It is the moral equivalent to requiring that an Error be checked before being assigned to. I've updated the patch to require that.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57618