[PATCH] D153066: [AppleTables] Implement iterator over all entries in table

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:259
+  /// one that doesn't. The nameless version is substantially faster.
+  template <bool StoreName = true> class Iterator : MaybeWithName<StoreName> {
+    constexpr static auto EndMarker = std::numeric_limits<uint64_t>::max();
----------------
Can you use any of the iterator helpers we have to ensure the whole iterator contract is implemented easily?


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:297-303
+      if constexpr (StoreName) {
+        std::optional<StringRef> MaybeStr =
+            Current.Table.readStringFromStrSection(*StrOffset);
+        if (!MaybeStr)
+          return setToEnd();
+        this->Name = *MaybeStr;
+      }
----------------
Rather than template metaprogramming - could this be done lazily? (store StrOffset as a member of some Entry type (either the current one, or some wrapper) and a member function of that entry could do the work on-request?


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:328-331
+    decltype(auto) operator*() const {
+      assert(!isEnd() && "dereferencing end iterator");
+      if constexpr (StoreName)
+        return std::make_pair(Current, this->Name);
----------------
I don't think it's valid for forward iterators to return by value - I think yo uhave to create a pair as a member and return a reference to that. So that op-> can be implemented (since it needs to return a pointer).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153066