Re: [RFA] Integrated hashtables for compiler + cpplib

["Zack Weinberg" <zackw at stanford dot edu>]

On Fri, May 18, 2001 at 06:47:58PM +0100, Neil Booth wrote:

I would have commented on the preliminary patches you sent me but I
was out all last evening and didn't get to them in time.

The general idea looks good but I have problems with some of the
implementation.

Am I confused, or are you calling init_stringpool twice for C front
ends? (once from cppinit.c and once from toplev.c)  Same for
set_identifier_size (which is harmless, but confusing).

I don't like the #ifdef STANDALONE_CPP.  I think that stringpool.c
should be split in two, instead.  Leave the standalone code in
cpphash.c and wrap it from stringpool.c; do the extra work needed for
integrated mode there.  

It's okay if we then need to link cpphash.o into every front end, as
long as it doesn't drag in the rest of cpplib.

This would also make it very easy to avoid breaking cpplib's ability
to have multiple reader objects active at the same time.  cpphash.o
refers to a reader object, and stringpool.o provides a dummy one to
front ends that don't need the whole megillah.  stringpool.o is
replaced by c-stringpool.o for C, which does the complete integration
dance.

And it avoids the other problem you've created, where libcpp.a cannot
be used without the user providing a hash table implementation.
Instead, we put the basic hash table into the library and let front
ends extend it.

> +  /* Proxy callbacks from stringpool.c.  */
> +  PTR void_ptr;
> +  cpp_cb callback;

These have got to be the least informative structure-field names I've
ever seen.  What does this callback *do*?  Ideally I could tell just
from the field name.  I suspect that the pointer is an opaque data
object provided to the callback; again, the field name should say so
(whatever_data, where 'whatever' is an appropriate name for the
callback function, should do fine)

It's not obvious to me what the callback does from the places where
it's used, either.

> +get_identifier2 (text, length)

Barf.  Let's not make *more* similar functions with no useful
distinction in their names.

get_identifier_with_len (text, len) maybe?

> +make_identifier (node)

Needs a comment and possibly a more descriptive name.  This turns a
phase 4 (preprocessing) identifier into a phase 7 (compilation)
identifier, I *think*.  If so,

/* NODE is an identifier known to the preprocessor.  Make it known
   to the C front end as well.  */

or something like that.

This routine is a good example of why the STANDALONE_CPP trick is bad;
I don't believe it will ever be called in the S_C case, but you're
leaving it in the object module anyway.

> +struct tree_common
> +{

As long as you're going to move this, mind incorporating my patch to
make the missing bits in the flag word accessible?
(http://gcc.gnu.org/ml/gcc-patches/2001-05/msg01149.html) 

-- 
zw        This is, no doubt, the rational strategy; quite possibly the
          only one that will work.  But it ignores the exigiencies of
          the tenure system and is therefore impractical.
          	-- Jerry Fodor, _The Mind Doesn't Work That Way_