This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA] comdat types


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]