Re: [PATCH] C++ patches

[Daniel Berlin <dberlin at redhat dot com>]

Andrew Cagney <ac131313@cygnus.com> writes:

> Kevin Buettner wrote:
> > 
> > On Sep 6,  9:35pm, Daniel Berlin wrote:
> > 
> > > I've rerun indent on the files this affects, because the formatting has
> > > gotten way out of whack with the GNU standards.
> > > I hope nobody minds, if somebody does, i'll undo the whitespace changes in
> > > my patch
> > > (diff -c3pwBb, remove the unbreaking of lines it did).
> > 
> > I think the generally agreed upon practice is to do two separate
> > commits; the first being the actual changes you made and the second
> > being the reformatting.  This way it's easier to review the first
> > patch.
> 
> Either order.  The main thing is to keep indentation and real codeing
> changes separate.  A maintainer shouldn't be presented with a patch that
> contains a combination of both.  Reviewing it is impossible :-(.

Yup.
Dunno what i was thinking.
Brain slipped.

> 
> Don't forget to clean the diff up a little (strip out the ChangeLog etc)
> next time :-)
> 
> > > Please note we now have one memory leak in lookup_symbol. The demangler
> > > gives us a buffer we never free. I'd need to free it at every exit point
> > > from the function, of which we have 12 right now, all of which end almost
> > > the exact same way (most return fixup_symbol_section(sym), a few just
> > > return sym. Is this incorrect?).
> > >
> > > I'm thinking that i'll change it so we just have a label at the end, and
> > > 12 gotos, rather than 12 exit points, and do the cleanup of the
> > > buffer there.
> > > It would make it about 50x easier for someone who needed to do something
> > > that requires cleanup when we exit.
> > 
> > I think gotos in this case are preferable.
> 
> Gotos are bad.  M'kay?  We've so far spent three years trying to
> eliminate all the goto's in WFI, adding more will just make me depressed
> :-)
> 
> Why not push the ALL_SYMTABS() loop into a sub function parameterized by
> ``name'' - I'm assuming that it is the loop that has the N different
> exit points?  That way you get your goto without actually having a
> goto...  Hmm, I suspect that ``name'' will need a cleanup.  Throw an
> error and, regardless of the gotos, it will leak memory.
I'm not staring at the code right now, but IIRC, part of the problem
was that they aren't all inside one loop.

I have IMHO a better solution, like the above. I'll split
lookup_symbol into lookup_symbol and lookup_symbol_aux, do the case
sensitivity/demangling in lookup_symbol, then call lookup_symbol_aux,
do the cleanup, and retunr whatever aux gave us.
Is this acceptable?

> 
> Keep it up.
> 
> 	Andrew