Re: [rfa] symbol hashing, part 2/n - ALL_BLOCK_SYMBOLS

[Daniel Jacobowitz <drow@mvista.com>]

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.

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.

> 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.

> 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.

> 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.

> 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.

> I cannot spot any others, can you?

Nope.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer