Re: PATCH: Altivec context-sensitive keywords

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Ben" == Ben Elliston <bje@au1.ibm.com> writes:

Ben> This patch is a revised attempt to implement context-sensitive keywords
Ben> for the PowerPC port.  There are some libcpp changes, some rs6000
Ben> backend changes, and some minor testsuite changes.

I looked at this.  BTW for those following along, the previous patch
is here:

    http://gcc.gnu.org/ml/gcc-patches/2007-12/msg00306.html

Ben> To overcome many of the concerns about the interaction of
Ben> context-sensitive keywords with ISO C compliance, the feature is now
Ben> controlled by !flag_iso.

I read through this a bit but I don't see how the new patch addresses
test #3 from Joseph:

    http://gcc.gnu.org/ml/gcc-patches/2007-12/msg00339.html

Would you mind explaining that?  I feel like I must be missing
something.

Ben> +static GTY(()) cpp_hashnode *__vector_keyword;

Does this work?  I thought that since cpp_hashnode was embedded in
another structure, you need special marking magic to make the GC work
here.

Maybe it does not matter since we only turn on identifier GC after
parsing is complete.  If so, that seems a bit fragile to me.

Ben> +  /* Keywords without two leading underscores are context-sensitive,
Ben> +     and hence implemented as conditional macros, controlled by the
Ben> +     rs6000_macro_to_expand() function above.  */

s/above/below/

Ben> +  static bool vector_keywords_init = false;
Ben> +  if (!vector_keywords_init)
Ben> +    {
Ben> +      init_vector_keywords (pfile);
Ben> +      vector_keywords_init = true;
Ben> +    }

I think this must be unnecessary... init_vector_keywords is called
elsewhere, and IIUC, rs6000_macro_to_expand can only be called for
conditional macros -- and if we have not called init_vector_keywords
already, then there won't be any of these.

Ben> +void
Ben> +_cpp_backup_tokens_direct (cpp_reader *pfile, unsigned int count)

I would prefer, if possible, not to have new public cpp functions that
frob cpp internal state like this.  The cpp code is pretty obscure
already; code paths like this make it even harder to reason about.

At the very least I think public methods should not have a leading "_".

Ben> @@ -630,7 +635,7 @@ struct cpp_hashnode GTY(())
Ben>  					   Otherwise, a NODE_OPERATOR.  */
Ben>    unsigned char rid_code;		/* Rid code - for front ends.  */
Ben>    ENUM_BITFIELD(node_type) type : 8;	/* CPP node type.  */
Ben> -  unsigned char flags;			/* CPP flags.  */
Ben> +  unsigned int flags;			/* CPP flags.  */

Do you know how this affects memory use?  I don't know how many
cpp_hashnodes are created ... this looks like a tightly-packed
structure, and I am wary about expanding it.

We could make a new NT_CONDITIONAL_MACRO type (maybe hard).  Or, I
think we can steal a bit from the type field -- if this does not make
the code big and/or slow.

Ben> +/* Look ahead in the input stream.  */
Ben> +const cpp_token *
Ben> +_cpp_peek_token (cpp_reader *pfile, int index)

Remove the leading "_".

It would be good to have test cases for combinations of vector and the
other conditional macros at EOF and in other weird places (#pragma or
_Pragma comes to mind).  I've fixed a number of crashes and whatnot
related to buffer manipulation at these boundaries... 


As long as the context-sensitivity remains lexical, as in this patch,
I'm ok with it.  But note that, on the incremental compiler branch,
the C compiler lexes the entire CU at once (as the C++ FE does on
trunk).  So, if hooks into the C parser are needed, this approach will
break the incremental compiler.

Since I'd prefer to avoid this, if parser hooks are needed, I would
prefer to see context-sensitive keywords.  I realize they are
unpopular, but at least they would support lexing AOT.

FWIW I think context-sensitive keywords would be easier to understand
even than the current patch, simply because they would be explicit
code in the parser rather than a callback buried in cpp.  Also, this
approach would not require new state-manipulating public cpp
functions.

Tom