From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11873 invoked by alias); 10 Jun 2003 16:43:42 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 11807 invoked from network); 10 Jun 2003 16:43:41 -0000 Received: from unknown (HELO smtp8.Stanford.EDU) (171.67.16.35) by sources.redhat.com with SMTP; 10 Jun 2003 16:43:41 -0000 Received: (from root@localhost) by smtp8.Stanford.EDU (8.12.9/8.12.9) id h5AGhdnl004586 for gdb-patches@sources.redhat.com; Tue, 10 Jun 2003 09:43:39 -0700 (PDT) Received: from jackfruit.Stanford.EDU (jackfruit.Stanford.EDU [171.64.38.136]) by smtp8.Stanford.EDU (8.12.9/8.12.9) with ESMTP id h5AGhah5004577; Tue, 10 Jun 2003 09:43:37 -0700 (PDT) Received: (from carlton@localhost) by jackfruit.Stanford.EDU (8.11.6/8.11.6) id h5AGha704872; Tue, 10 Jun 2003 09:43:36 -0700 X-Authentication-Warning: jackfruit.Stanford.EDU: carlton set sender to carlton@math.stanford.edu using -f To: Elena Zannoni Cc: gdb-patches@sources.redhat.com, Jim Blandy Subject: Re: [rfa] struct dictionary References: <16101.3585.739945.993550@localhost.redhat.com> From: David Carlton Date: Tue, 10 Jun 2003 16:43:00 -0000 In-Reply-To: <16101.3585.739945.993550@localhost.redhat.com> Message-ID: User-Agent: Gnus/5.0808 (Gnus v5.8.8) XEmacs/21.4 (Common Lisp) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-SW-Source: 2003-06/txt/msg00328.txt.bz2 On Mon, 9 Jun 2003 18:45:21 -0400, Elena Zannoni 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. >> @@ -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. 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... David Carlton carlton@math.stanford.edu