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

[Elena Zannoni <ezannoni at cygnus dot com>]

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