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

[Felipe de Azevedo Piovezan via Phabricator via llvm-commits]

fdeazeve 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");
+  }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152159