[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:252
+
+    void setOffset(uint32_t NewOffset) { StrOffset = NewOffset; }
+    std::optional<StringRef> readName() const {
----------------
If `StrOffset` is going to be public, probably could skip the accessor and mutate it directly?


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:248
 
+  struct EntryWithName final : Entry {
+    EntryWithName(const AppleAcceleratorTable &Table)
----------------
fdeazeve wrote:
> dblaikie wrote:
> > Ah, sorry, I probably wouldn't complicate this by deriving, needing a virtual dtor, etc.
> > 
> > Maybe composition? EntryWithName could be closer to the std::pair (but, yeah, it needs to be a bit smarter) than this - contains an Entry, the offset and a function to get the name.
> It seems that composition leaks a lot of the implementation to users of this class. For example, they will need to do `Entry.BaseEntry.getDIESectionOffset` instead of just `Entry.getDIESectionOffset`. To avoid this, we would need to copy all public methods of `Entry` and implement them by calling the corresponding method on the `Entry` member... but that's just re-implementing inheritance manually.
> 
> Note that the base class `Entry` still derives from a pure virtual class, even if we remove this new inheritance; the only reason the compiler was not giving a dtor warning before is because `Entry` was marked as `final`.
> 
> IMO the more artificial part of this hierarchy is the pure virtual class, which AFAICT is not taken advantage of (I couldn't find any virtual uses of them). I believe the original goal was that Apple tables and the Dwarf 5 tables could share an interface, but maybe it doesn't have to be a virtual interface, instead some sort of CRTP. 
> 
> All that said, for now I have moved to composition instead of inheritance. If we feel like this becomes too cumbersome, we can re-evaluate.
Fair enough - I still tend a bit towards composition, though perhaps if you want to cleanup the virtual base, if it turns out it hasn't been useful - then maybe derivation isn't so bad? 


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:263
+    Entry Current;
+    uint64_t Offset;
+    uint32_t NumEntriesToCome;
----------------
fdeazeve wrote:
> 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? 
yeah, I'd second the suggestion - I'd tend towards a member initializer over an init list, seems to simplify things a bit - if another ctor is added, if it's a clear default state for the member, etc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153066