[PATCH] D152159: [AppleAccelTable][NFC] Refactor iterator class

[David Blaikie via Phabricator via llvm-commits]

dblaikie added subscribers: lhames, dblaikie.
dblaikie added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:279
+        FormValue.extractValue(Table.AccelSection, Offset, Table.FormParams);
+    assert(ExtractWorked && "Failed to extract value from hash data entry");
+  }
----------------
fdeazeve wrote:
> aprantl wrote:
> > What does it mean for `ExtractWorked` to be false?
> > If this can be triggered by malformed input, even an asserts-enabled parser needs to handle it gracefully. Asserts should only be used for internal consistency checks that may never fail.
> >If this can be triggered by malformed input, even an asserts-enabled parser needs to handle it gracefully. 
> 
> That's what I mean in the commit message with:
> 
> > We maintain the already existing assumption that failures never happen, but now
> > make it explicit with an assert.
> 
> Today, if we fail to extract, we're going to start producing completely incorrect data and be silent about it. Imagine one of the iteration fails: the offset doesn't get updated and the next iteration is going to re-use the offset of the previous iteration.
> 
> IMO we're not handling it gracefully today, that's why I added the assert. But I don't feel strongly about this (just a bit :) )
> 
> I think the only way to fix this correctly is to remove the iterator abstraction and use callbacks instead. Non-end iterators should never fail the dereference operator, but data extracts are inherently a "fail-able" abstraction.
I believe @lhames and I spoke about fallible iterators previously & he's used some iterator abstractions with fallibility (which can be nicer than a callback - for instance, allowing early exit or other novel iteration sequences that a callback API might not provide in a general way) - by having the iterator terminate early (when it hits an error, it becomes == end) and having a member you have to inspect after the fact to query for the error state/if it's failed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152159