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


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:263
+    Entry Current;
+    uint64_t Offset;
+    uint32_t NumEntriesToCome;
----------------
dblaikie wrote:
> 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
Sounds good, I'll initialize this to the end marker.

I can see why that is appealing, but personally believe it gives the false security that things are initialized properly. To use your example of a new ctor, consider the case someone forgets to initialize the member: no tool will catch this when member initialization was used. But tools exist to catch use of uninitialized variables/members (e.g. `-Wuninitialized`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153066