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
