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

Felipe de Azevedo Piovezan via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 6 04:02:01 PDT 2023

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))
+    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

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list