[PATCH] D151989: [AppleAccelTable][NFC] Refactor equal_range code

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

fdeazeve added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:343
+  for (uint64_t DataOffset = *MaybeDataOffset;
+       DataOffset < AccelSection.size();) {
+    std::optional<uint32_t> StrOffset = readStringOffsetAt(DataOffset);
----------------
aprantl wrote:
> This isn't really a loop, is it?
> Maybe just use an if () statement and convert this to another early exit?
That's right. It *should* be (if we were actually checking all hash collisions) but once we rewrite the algorithm this way it becomes obvious that there is a bug. I can replace the `for` with an `if` for now, but we will re-instate the `for` once the bug is fixed


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:374
+    if (!MaybeHash || !wouldHashBeInBucket(*MaybeHash, BucketIdx))
       break;
+    if (*MaybeHash == HashToFind)
----------------
aprantl wrote:
> Feel free to ignore this suggestion, but we could consider returning an error with the offset of the failure here?
You mean the failure we got from attempting to read the `HashIdx`-th hash? (i.e. the fact that `MaybeHash` was `nullopt`?)

I think if we were to try and expose extraction errors with an `Error` instead of an `optional`, this would propagate up to all functions of this file. So it's something we can consider but I'd advise to do it separately, as we already have quite a few cleanups being done in the stack


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151989