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

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

fdeazeve 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();
----------------
dblaikie wrote:
> Can you use any of the iterator helpers we have to ensure the whole iterator contract is implemented easily?
Oh I didn't know these exist, I'll have a look!


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:263
+    Entry Current;
+    uint64_t Offset;
+    uint32_t NumEntriesToCome;
----------------
aprantl wrote:
> should this be default initialized?
I'm not sure I follow the suggestion: the one and only constructor of the class ensures it is initialized properly. Is there some concern about initialization here? 


================
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;
+      }
----------------
dblaikie wrote:
> 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?
That's an interesting one. Note that this *is* done lazily: we only do it if someone calls `all_entries_with_names` (instead of `all_entries`) which presumably means they are interested in the name.

That said we could add an extra member to the `Entry` class which would substantially simplify code and also enable me to address the other concern you had about operator `->`! I'll give this a try.


================
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);
----------------
dblaikie wrote:
> 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).
Good point, let me give this a try


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153066