From mboxrd@z Thu Jan 1 00:00:00 1970 From: Elena Zannoni To: Daniel Jacobowitz Cc: Elena Zannoni , gdb-patches@sources.redhat.com Subject: Re: [rfa] symbol hashing, part 2/n - ALL_BLOCK_SYMBOLS Date: Fri, 12 Oct 2001 08:49:00 -0000 Message-id: <15303.4807.801121.915108@krustylu.cygnus.com> References: <20011009123457.A28534@nevyn.them.org> <15302.12121.778853.77938@krustylu.cygnus.com> <20011011195450.A22256@nevyn.them.org> X-SW-Source: 2001-10/msg00166.html Daniel Jacobowitz writes: > On Thu, Oct 11, 2001 at 07:46:33PM -0400, Elena Zannoni wrote: > > > > Daniel, > > Thanks so much for doing this. It makes it so much easier. > > > > Yes, I looked ths over and it seems to work, except that I would really > > prefer the change to printcmd.c split in two. The first bit to > > rationalize that "if (func)..." code. This would have with it all > > the indentation changes as well. The code as it is now doesn't really > > make much sense. So, that looks a good change to me. But it has nothing > > to do with the new macro. After that change is in, you can introduce > > the macro in printcmd.c w/o having all the indent changes. > > It also makes it easier to distinguish a no-op change (the macro) from > > the other one. > > OK. Would you prefer I resubmit this patch broken up further, then? > I could do that. No need to resubmit. Just check it in as 2 patches instead of 1. So that I can do cvs diffs between versions of the file later, if there is an issue. > > There's a double-edged sword here; every patch in this sequence except > for the hashing change is predicated on the previous patches. So while > I understand that breaking them up does make reviewing much easier, > with the current length of the patch review cycle, every time I > decompose this further I add two or three more days to its eventual > (hopeful) approval. I'm sure you can understand that it's a little > frustrating. > In theory, the case at hand shouldn't even occur. I myself have done on the order of half a dozen commits to the same file in the same day, just to break my changes into minimal pieces. I personally tend to strip down patches to their bare bones. I agree though that it makes it harder to keep the various copies of the files in synch. But as I said, I don't care about you resubmitting this patch. > > The cahnge is printcmd.c needs to delete also the > > sym = BLOCK_SYM (b, i); > > line. > > ... that's what I get for moving too fast between trees. Thanks for noticing. > > > > > [Note that I don't maintain printcmd.c, so, I should just shut up :-)] > > > > I applied your patch as is to my sources, and did a grep for BLOCK_SYM, > > and found a few more for loops that could be converted: > > > > > > This one in buildsym.c: > > line ~280: > > > > struct symbol *sym; > > for (i = 0; i < BLOCK_NSYMS (block); i++) > > { > > sym = BLOCK_SYM (block, i); > > I could change this one too. It's from a case where the symbols will > never be hashable (i.e. order-sensitive), so I didn't touch it the > first time through. Consistency is good, though. Yes, I would prefer to change this too. > > > And this one in symtab.c: > > line ~1500: > > > > top = BLOCK_NSYMS (block); > > for (bot = 0; bot < top; bot++) > > { > > sym = BLOCK_SYM (block, bot); > > > > And this one's #if 0'd out. I could fix it (just the ALL_BLOCK_SYMBOLS > change, I mean), or delete it per the comment above it. > Ah, I didn't notice this. Just leave it then. How about this other one? in symtab.c ~line 1254 top = BLOCK_NSYMS (block); while (bot < top) { sym = BLOCK_SYM (block, bot); if (SYMBOL_NAMESPACE (sym) == namespace && SYMBOL_MATCHES_NAME (sym, name)) { return sym; } bot++; } > > Another one is in mi/mi-stack-cmd.c. > > There are some also in gdbtk/generic/gdbtk-cmds.c and gdbtk-stack.c. > > These I missed; I didn't think to check further down than the top > level. I'll get them when I repost this, which it looks like I'll need > to do. > No, just post the new changes only. > > Some tricky ones are left, for a second pass. In mdebugread.c: it is > > actually a while loop, in mylookup_symbol, similarly in coffread.c: > > patch_opaque_types(), the for loop is reversed. > > (Were these in your original patch? I don't remember). > > The one in coffread worries me; I can not tell from the comments if it > is order-sensitive or not. The one in mdebugread does not, because > mdebugread does dastardly things to blocks. I deliberately left those > unhashed. Hmmm, maybe we should leave the coffread one alone. But I think we should use the macro in mdebugread.c. > > > I cannot spot any others, can you? > > Nope. > Ok, good. Elena > -- > Daniel Jacobowitz Carnegie Mellon University > MontaVista Software Debian GNU/Linux Developer