Re: [RFA] comdat types

[Doug Evans <dje@google.com>]

On Thu, Jun 25, 2009 at 12:46 PM, Tom Tromey<tromey@redhat.com> wrote:
> I'm curious about one thing on this page, and I thought I'd take the
> opportunity to ask.  DW_TAG_type_unit may have a DW_AT_language child.
> But, the language is not mentioned in the suggested method for
> computing a type's signature.  This seems strange to me: either the
> language matters (in which case, it seems like it ought to be in the
> signature); or the language does not matter, in which case, why
> mention it?

I'll leave this for Cary.

> One concern I have is whether there is any chance that the
> specification will change between now and when DWARF-4 is published.

Cary?

> I do have a few nits.
>
> Doug> +  types_htab = htab_create_alloc_ex (41,
> Doug> +                              hash_type_signature,
> Doug> +                              eq_type_signature,
> Doug> +                              NULL,
> Doug> +                              &objfile->objfile_obstack,
> Doug> +                              hashtab_obstack_allocate,
> Doug> +                              dummy_obstack_deallocate);
>
> This is just a side note -- I've seen a few hash tables allocated on
> obstacks.  Doesn't resizing the table waste memory?

IIRC I use the noresize traversal routine.
It's not ideal, no.

> Too bad we don't have allocation pools instead of obstacks.  I suppose
> in this specific case we could use the objfile data machinery to
> deallocate hash tables.

If that's ok, I'll do that.

> Doug> +  if (dwarf2_die_debug)
> Doug> +    {
> Doug> +      fprintf_unfiltered (gdb_stdlog, "Signatured types:\n");
> Doug> +    }
>
> Over-bracing.  There's a fair amount of this in the patch.

One of the style rules I'm less fond of (and see others are too from
scans of gdb, is this a rule or a guideline?).
[I'm reminded of Pirates of the Caribbean, "They're more like
guidelines anyway." :-)]
But ok.

> Doug> +  if (this_cu->from_debug_types)
> Doug> +    {
> Doug> +      /* ??? How come this is for .debug_types only?  */
> Doug> +      this_cu->offset = cu.header.offset;
> Doug> +      this_cu->length = cu.header.length + cu.header.initial_length_size;
>
> Daniel has asked before for "no new FIXMEs".  You can't escape this by
> spelling it "???" :-)

Well, that one was an oversight (these patches drag on and my eyes
tend to glaze over ...).
While as a general rule I don't disagree, it's kinda odd to see others
add new functionality with open issues.

> Seriously, I think this is a valid concern and I would like some
> definitive resolution to the various "???" comments.
>
> Doug> + NotFound:
>
> Should be "not_found".  There's a couple of these.

fwiw I like StudlyCaps for labels:  How many labels should there be in
one's code?
--> As absolutely few as possible.
Same with StudlyCaps. :-)
There are existing uses of StudlyCaps for labels (albeit isolated to
one file) so I went with that.
But ok.