This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [rfa] symbol hashing, part 2/n - ALL_BLOCK_SYMBOLS
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