Re: [PATCH]: C++ mangling patch that is about to be committed

[Kevin Buettner <kevinb at cygnus dot com>]

On Oct 10,  8:33pm, Daniel Berlin wrote:

> gdb_mangle_name is a perfect example.
> It has nothing to do with symbols.
> All it does is take a type structure, a method id, and a signature,
> and build a new mangled name for it.
> It involves magical knowledge of how g++ mangles names.
> It doesn't even have a struct symbol in it, or anywhere near it.
> It isn't called except from a few C++ specific routines (cp-valprint
> or typeprint calls it, and one other place that escapes the mind).
> Of course, it's in symtab.c.
> 
> Are you starting to see my point?
> The people i have to get approval for are really not qualified to
> examine the patches.
> No offense, but taking you as an example, it took you a while to
> figure out what my changes were doing, and they aren't obsfuscated
> code.

Hmm... "really not qualified"...?

Daniel, I don't think that this choice of words is the way to win
friends and influence people.

If I read your implications correctly, then I am in complete
disagreement with you regarding Elena's qualifications.  Specifically,
when I've asked her for help in this area (gdb symbol tables), she's
always provided good and useful answers.

Prior to that though (in your message), I was starting to see your
point.  It's like this...  there are times when someone relatively
unfamiliar with a piece of code can go into it to fix a bug, or work
on an optimization, or add a new feature, or whatever.  At the end of
that time, whether it be a matter of hours or a number of days, I
think it is frequently the case that that person who was heretofore
unfamiliar with the code in question will have learned enough to rival
the maintainer's knowledge of the code, particularly if said
maintainer is also working on a bunch of other stuff at the same time.

But this is a temporary situation.  Or it should be at any rate
because when you provide a patch for someone to review, I've
always thought it best to:

 1) Describe the problem that your patch is trying to solve.

 2) Show (via a detailed analysis if necessary) why the current
    code is broken.

 3) Likewise, describe why your patch fixes the problem.

 4) Provide an analysis of the side effects (if any) of your
    patch, or other areas where it may be deficient.

In other words, you should attempt to pass along the knowledge that
you've gained while working on a certain piece of code.

You should also provide a summary of any domain specific knowledge
(e.g, g++ name mangling behavior) which you have that bears upon the
problem.  It's okay to provide a reference if it's incredibly
detailed.  (For example, see
http://sources.redhat.com/ml/gdb-patches/2000-08/msg00075.html in
which I provide a reference to a DWARF2 draft document.  Note,
however, that I summarize the salient points in the message proper.)

I know that this creates (substantially) more work for the patch
submitter.  But I also believe it creates higher quality patches.  I
do not always take the time to do everything that I described above,
but when I do, I'm forced to look more critically at my changes.  I
will frequently find problems with them in the process of writing my
analysis.  Even if I find no problems, the analysis will hopefully
give the maintainer the necessary background to see why the proposed
changes are necessary.  My motives are not completely altruistic; I've
discovered that providing a cogent analysis often means that my
patches are approved quicker which cuts down on the merge and retest
time.  (We all *do* test our changes again prior to committing them,
don't we?)  Also, I tend to forget these details relatively quickly,
so writing them down is a way of preserving this knowledge for myself
as well.

In my opinion, it is also useful for maintainers to go through this
exercise for patches in their own area since these patches are still
subject to peer review.

Kevin