On Tue, Oct 1, 2024 at 2:10 PM Petr Pavlu <petr.pavlu@xxxxxxxx> wrote: > > On 9/23/24 20:18, Sami Tolvanen wrote: > > +static void update_fqn(struct die *cache, Dwarf_Die *die) > > +{ > > + if (!cache->fqn) > > + cache->fqn = get_fqn(die) ?: ""; > > +} > > When is update_fqn() called with cache->fqn being already non-NULL? In patch 16, because we need the name before process_fqn() is called, and if we end up outputting the name after that, cache->fqn is already non-NULL. > In general, I find handling of cache->fqn slightly confusing, mostly > because the member has three states: NULL initially, > a statically-allocated empty string if the DIE doesn't have a name and > a dynamically-allocated non-zero-length string otherwise. > > I wonder if it would be possible to reduce it to two states: NULL > initially and when the DIE doesn't have a name, or a regular string. I also thought about it, but using an empty string to represent an unnamed DIE and NULL to represent an uninitialized value seemed the most reasonable option. > > +static inline const char *die_state_name(enum die_state state) > > +{ > > + switch (state) { > > + default: > > + CASE_CONST_TO_STR(DIE_INCOMPLETE) > > + CASE_CONST_TO_STR(DIE_COMPLETE) > > + } > > Nit: I suggest to move the default case out of the switch statement: > > switch (state) { > CASE_CONST_TO_STR(DIE_INCOMPLETE) > CASE_CONST_TO_STR(DIE_COMPLETE) > } > error("unexpected die_state: %d", state); > > This way, if someone adds a new value in die_state and forgets to handle > it in die_state_name(), they get a compiler warning.. or a runtime error > later. Sure, I'll change this. Sami