[Mesa-dev] [GLSL] defined expressions

[José Fonseca]

On Fri, 2010-12-03 at 15:37 -0800, Carl Worth wrote:
> On Fri, 3 Dec 2010 13:34:09 -0800, Kenneth Graunke <kenneth at whitecape.org> wrote:
> > On Friday 03 December 2010 08:01:06 José Fonseca wrote:
> > >   parser->space_tokens = 1;
> > > 
> > > statement conditional_tokens stanza -- it causes space tokens to be
> > > emitted halfway before reaching the DEFINED token.
> > 
> > Indeed.  I'm not sure why conditional_tokens sets this.  That code was pretty 
> > much copy & pasted from pp_tokens, too, so I'm wondering if it can be removed 
> > from there as well.
> > 
> > Carl, can we safely remove this line from conditional_tokens?  What about 
> > pp_tokens?
> 
> The current glcpp test suite (glcpp-test) says it's safe to remove it
> From conditional_tokens. Take that for what it's worth I guess...
> 
> It's definitely not safe to remove it from pp_tokens, (unless you also
> make some other changes). If you just remove that you'll find that
> something like:
> 
> 	#define foo bar baz
>         foo
> 
> will result in "barbaz" rather than "bar baz" as desired.
> 
> One option here that might simplify the implementation considerably is
> to not include space tokens in the replacement list like this, but to
> instead simply insert spaces between all tokens when printing the
> resulting stream.
> 
> That would cause some minor expansion of the results. For example input
> of:
> 		(this,that)
> would become:
> 		( this , that )
> 
> But that probably doesn't really harm much.
> 
> During development of the preprocessor it has been *very* convenient to
> not do expansion like this so that results could be verified based on
> direct comparison with output from "gcc -E", (and attempts to test with
> diff but ignoring amounts of whitespace proved error prone). Now that
> the test suite uses its own files of expected output, this issue
> probably doesn't exist.
> 
> > > I don't quite understand space_tokens' role here, or how it is supposed
> > > to work. I confess I'm not very familiar with bison/flex inner-workings:
> > > I though that the tokenizer was producing tokens quite ahead of the
> > > parser, so the parser couldn't change the behavior of the tokenizer in
> > > flight -- but perhaps I'm mixing up with other lexel/grammar generation
> > > tools.
> 
> The parser can exert some influence over the tokenizer. But doing this
> reliably is difficult since the tokenizer can do read-ahead. I
> definitely agree that it would be greatly preferable to have the
> tokenizer and parser more cleanly separated. But various "messy" aspects
> of the C preprocessor language do make this difficult.
> 
> > Apparently it can.  I believe Ian's assessment is right.
> 
> The distinction between "#define foo ()" and "#define foo()" is a case
> where the C preprocessor language makes whitespace significant. But the
> current space_tokens control is not used for this. Instead the lexer is
> currently handling this distinction itself.
> 
> So space_tokens is really extra code to try to get really clean output,
> (without extra space expansion as shown above). If removing space_tokens
> makes the implementation much cleaner then the slightly less clean
> output might be very well worth it.
> 
> I'll leave that for you to decide, Ken.

I suppose it is possible for space to be significant on a conditional
expression, e.g.,

  #if foo ()

vs 

  #if foo


If there isn't an obvious cleaner solution, would you mind I commit my
original patch:

        diff --git a/src/glsl/glcpp/glcpp-parse.y b/src/glsl/glcpp/glcpp-parse.y
        index 8475e08..3b84c06 100644
        --- a/src/glsl/glcpp/glcpp-parse.y
        +++ b/src/glsl/glcpp/glcpp-parse.y
        @@ -462,6 +462,10 @@ conditional_token:
                        int v = hash_table_find (parser->defines, $2) ? 1 : 0;
                        $$ = _token_create_ival (parser, INTEGER, v);
                }
        +|      DEFINED SPACE IDENTIFIER {
        +               int v = hash_table_find (parser->defines, $3) ? 1 : 0;
        +               $$ = _token_create_ival (parser, INTEGER, v);
        +       }
         |      DEFINED '(' IDENTIFIER ')' {
                        int v = hash_table_find (parser->defines, $3) ? 1 : 0;
                        $$ = _token_create_ival (parser, INTEGER, v);
        
It doesn't makes things any worse, and it would allow us to focus on
other problems.

Jose