Re: RFA: abstract C++ ABI dependencies

[Daniel Berlin <dan at cgsoftware dot com>]

Eli Zaretskii <eliz@is.elta.co.il> writes:

> On 25 Apr 2001, Daniel Berlin wrote:
> 
> > Eli Zaretskii <eliz@is.elta.co.il> writes:
> > 
> > > On 24 Apr 2001, Jim Blandy wrote:
> > > 
> > > > > 	    * c-typeprint.c, c-valprint.c, dbxread.c, eval.c, gdbtypes.c,
> > > > > 	    jv-typeprint.c, linespec.c, symtab.c, typeprint.c, valops.c:
> > > > > 	    #include "cp-abi.h".
> > > > > 
> > > > > For c-valprint.c, eval.c, typeprint.c, and valops.c, it is unclear to
> > > > > me why you are including cp-abi.h.
> > > > 
> > > > Because they use functions which used to be declared elsewhere, but
> > > > are now declared in cp-abi.h.  I've clarified the ChangeLog entry.
> > > 
> > > Isn't it better to put such explanations in the files which include 
> > > cp-abi.h, rather than in a ChangeLog?
> > 
> > Errr, i don't see any other comments on header includes.
> 
> I see _lots_ of them.  Two examples (you can see them all with
> "grep '#include..*/\*' *.c"):
> 
>   utils.c:#include "inferior.h" /* for signed_pointer_to_address */
>   linux-thread.c:#include "gdb_wait.h" /* for WUNTRACED and __WCLONE flags */
> 
> But even if these comments were not present, that doesn't mean they 
> shouldn't be there.
> 
> > Why would you list the reasons for including a given header? 
> 
> Because people who read the code afterwards might ask themselves why
> was that header included, if the name of the header doesn't make that 
> obvious.
In this case, the header filename is pretty obvious, IMHO.
cp-abi.h includes C++ ABI related definitions.

> 
> This seems especially important since lately some of the maintainers are 
> actively terying to remove unneeded headers in order to  minimize 
> dependencies and slash the build time.


> 
> > If you want to know what's in the header, and you can't tell from the
> > top of the header itself, or the filename of the header, well, then,
> > there's probably something else wrong.
> 
> Maybe something _is_ wrong.  If so, the code should be reworked 
> accordingly.

This was done in this case. This is the result of the rework.

> 
> However, if, after all the deliberations, it is decided to leave the 
> header inclusion, and that header's name does not explain why it is 
> needed, like in the case in point (see David's question), then I think
> the explanation should be in the code, not in the ChangeLog.

This is not the case in what you are referring to.
The reality is that some things that didn't belong in other headers
and files, related to C++ ABI stuff, were moved into new headers and
files, and abstracted so that the old function *implementations* and
macros no longer existed or were no longer "public" (like the macros
related specifically to virtual function start offsets in the HP C++
ABI implementation, which are now in hpacc-abi.h).

This necessitated the inclusion of that new header.  In other words,
this is the case of comments should have been there before about
including gdbtypes.h and symtab.h for some C++ ABI stuff as well, and
we should now be able to remove them.

Since there are no comments there, there is nothing to remove.

and IMHO, saying 

#include <cp-abi.h> /* Include C++ ABI stuff */

is like saying

i++; /* Increment i */

If it's not immediately obvious what cp-abi.h does, the first line of
the file will give it to you.

So, basically, i'm saying that before, we should have had

#include <symtab.h> /* ... Get some C++ ABI related junk */
#include <gdbtypes.h> /* ... Get some other C++ ABI related junk */
#include <value.h> /* ... Get yet some more C++ ABI related junk */

or something, and been able to just replace all three lines with

#include <cp-abi.h> 

with no comment.


> 
> In other words, if there were no reason to have an explanation, why would 
> Jim feel he should provide one on the ChangeLog?

Because nobody else realized those macros were spread out across 2 (or
was it 3) header files, and had nothing to do with what the header
file purported to be, except me, because nobody had touched the hard
core C++ ABI stuff in at least year or so, except me?

And the functions were removed from the old headers, since they didn't
really fit in with what the rest of the functions were doing, and made
much more sense in the context of other functions (specifically,
functions that are used to implement support for a given C++ ABI).


Since this requires a lot of domain specific gdb knowledge to realize
this, in order for others to understand why it's done, you basically
need  two things:
1. A message explaining why it was done, on the mailing list (done at
some point in the past), so others understand.
2. A short reminder in the ChangeLog, saying why it was done.

Jim did #2, I think I did #1, IIRC.

-- 
"It's a small world, but I wouldn't want to have to paint it.
"-Steven Wright