Re: [rfa] struct dictionary

Elena Zannoni
2003-06-10T18:37:19Z

David Carlton writes:
> On Mon, 9 Jun 2003 18:45:21 -0400, Elena Zannoni <ezannoni@redhat.com> said:
> > David Carlton writes:
>
> > As usual, some comments. Could you check in the removal of the
> > sorting as one patch, and the dictionary as another?
>
> Will do.
>
> > See below for some things, but in general it's ok.
>
> Great!
>
> > I don't feel too comfortable with the Java stuff, are there any
> > tests that use that particular code? It sure looks safe to me
> > though.
>
> The Java testsuite might exercise it a little bit, but it might not.
> Once this goes in, I'll e-mail Tom Tromey to ask him to pound on it a
> bit.
>
> >> +/* Various vtbls that we'll actually use. */
> >> +
> >> +static const struct dict_vtbl dict_hashed_vtbl =
> >> + {
> >> + DICT_HASHED, free_obstack, add_symbol_nonexpandable,
> >> + iterator_first_hashed, iterator_next_hashed,
> >> + iter_name_first_hashed, iter_name_next_hashed,
> >> + };
> >> +
>
> > Could you do these one per line and put a comment with the field
> > name next to it (for easy grepping)? I am not too fond of the use
> > of vtable terminology, could you use 'vector' or something else?
>
> Sure.
>
> >> + $(infcall_h) $(dictionary.h)
>
> > dictionary_h
>
> Whoops, good eye!
>
> >> @@ -348,9 +309,11 @@ finish_block (struct symbol *symbol, str
> >> TYPE_FIELDS (ftype) = (struct field *)
> >> TYPE_ALLOC (ftype, nparams * sizeof (struct field));
> >>
> >> - for (i = iparams = 0; iparams < nparams; i++)
> >> + for (sym = dict_iterator_first (BLOCK_DICT (block), &iter),
> >> + iparams = 0;
> >> + iparams < nparams;
> >> + sym = dict_iterator_next (&iter))
>
> > could you use the macro ALL_BLOCK_SYMBOLS here? and move the check for
> > iparams inside the loop body?
>
> Sure, that makes sense. I was just going with the way it had been
> written before, but now that I look at it, it was only written that
> way for reasons that are no longer relevant: you want me to do
>
> ALL_BLOCK_SYMBOLS (block, iter, sym)
> {
> if (iparams == nparams)
> break;
>
> ....
> }
>
> And before this change, that wouldn't have worked, because
> ALL_BLOCK_SYMBOLS was secretly two nested loops, so you can't break
> out of them, but after my change it's just a single loop, so the break
> works just fine. Excellent.
>
> >> -static struct block *new_block (int);
> >> +static struct block *new_block (int function);
>
> > can this take an enum parameter with some meaningful names?
>
> Well, I'm just using it as a boolean, so it seems to me that doing an
> enum would be a little funny. How about I rename the argument to
> function_p, or is_function, or something like that? But if you'd
> prefer an enum, I could do something like
>
> enum block_type { FUNCTION, NON_FUNCTION };
>
> instead.

For me the real problem is to understand the code that does the
call. I don't know what 1 or 0 means at that point w/o going looking
for the function definition. And I lose my train of thoughts.
Don't really care too much because this is only in mdebugread.c though :-)

>
> >> @@ -1236,13 +1231,16 @@ parse_symbol (SYMR *sh, union aux_ext *a
> >>
> >> if (nparams > 0)
> >> {
> >> + struct dict_iterator iter;
> >> TYPE_NFIELDS (ftype) = nparams;
> >> TYPE_FIELDS (ftype) = (struct field *)
> >> TYPE_ALLOC (ftype, nparams * sizeof (struct field));
> >>
> >> - for (i = iparams = 0; iparams < nparams; i++)
> >> + for (sym = dict_iterator_first (BLOCK_DICT (b), &iter),
> >> + iparams = 0;
> >> + iparams < nparams;
> >> + sym = dict_iterator_next (&iter))
>
> > could you still use the macro here ALL_BLOCK_SYMBOLS and add an if
> > (iparams == nparams) break; inside the loop?
>
> Yes, presumably I can handle this like the buildsym case above.
>
> >> - register int i, j;
> >> + struct dict_iterator iter;
> >> + register int j;
>
> > no need for register.
>
> Right, sorry.
>
> >> @@ -493,6 +495,11 @@ dump_symtab (struct objfile *objfile, st
> >> fprintf_filtered (outfile, " under ");
> >> gdb_print_host_address (BLOCK_SUPERBLOCK (b), outfile);
> >> }
> >> +#if 0
> >> + /* NOTE: carlton/2003-04-28: If we really want to be able to
> >> + print out something here, we'll need to add an extra
> >> + dictionary method just for that purpose. */
> >> +
>
> > Hmmm, I think we should. We don't want to change gdb's behavior.
>
> Well, it's for a maint command, so we can really do whatever we want
> here. But if you think that's an important part of the info, I'll add
> the extra method.
>

thanks yes.

> I'll make those changes and commit it tomorrow afternoon (unless
> exam grading takes longer than I expect), if nobody complains; and
> I'll be spending all day Thursday doing GDB stuff as well, so I'll be
> available if I accidentally break anything...
>

ok, great. BTW, i have started setting up the same test framework
that Michael Chastain uses for his tests. I have done a preliminary
run, but I screwed up the combinations of gcc and binutils to test
with, so I'll get back to that tomorrow, and I should be able to help
with testing for gdb6.

elena

> David Carlton
> carlton@math.stanford.edu