Re: [RFA] Integrated hashtables for compiler + cpplib

[Neil Booth <neil at daikokuya dot demon dot co dot uk>]

Zack Weinberg wrote:-

> 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).

init_stringpool: yes, toplev.c calls it.  I also need to call it from
cpplib in case we're stand alone.  init_stringpool notices when it's
called more than once and ignores subsequent calls.  Not pretty, but
not that bad either.

set_identifier_size: No.  It needs to be called earlier than before,
since cpplib gets initialised earlier.  Otherwise nasty things (tm)
happen.  That's why I moved it.

Like I said, set_identifier_size has an life expectancy of about 3
days, anyway.  Don't worry about that part.

> 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.

Hmmm.  OK, I'll have a go, but it's not clear to me if it'll all fit
together.  You might lose your statistics, for example - I did my best
to preserve them in the patch I posted.

> 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.

? What's a megillah? :-)

> stringpool.o is replaced by c-stringpool.o for C, which does the
> complete integration dance.

Hmm.  So we have cpphash.o, stringpool.o and c-stringpool.o?  I'm not
clear what you're thinking of here, but I'll see what I can do.

> 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.

Yes, I didn't particularly like that bit myself.
 
> > +  /* 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.

This was a short-term hack to get round the lack of re-entrancy
(i.e. a pfile pointer) of stringpool.o.  It would've gone away with
the now-hypothetical follow-up patch.  callback was the real callback
requested by the cpplib client; cpplib used its own internal proxy to
convert the "void *" (in fact pfile) it passed to stringpool.c into
the "void *" (voidptr above) given to it by cpplib's client as the
"data" of the callback.  If you see what I mean.

Ugly, yes, but it would only have been there for a day or two anyway.

> get_identifier_with_len (text, len) maybe?

OK.

> > +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.

Well, it exists for whatever reason the distinction between
get_identifier and maybe_get_identifier exists.  I'm not entirely
clear on that, but I belive it's something to do with the compiler
avoiding outputting (debug? object?) information for stuff that isn't
used.  That distinction is the ERROR_MARK / IDENTIFIER_NODE
distinction we have now, and make_identifier converts from ERROR_MARK
to an IDENTIFIER_NODE.

I guess your suggested comment is a fair description.

> 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) 

I'll take a look.

Thanks for the feedback,

Neil.