Re: [rfa] struct dictionary

[Elena Zannoni <ezannoni at redhat dot com>]

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