[Mesa-dev] [PATCH 1/2] glcpp: Add a more descriptive comment for the SKIP state manipulation

[Ian Romanick]

Other than the code formatting comment below, this series is

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

On 12/19/2013 04:25 PM, Carl Worth wrote:
> Two things make this code confusing:
> 
> 	1. The uncharacteristic manipulation of lexer start state outside of
> 	   flex rules.
> 
> 	2. The confusing semantics of the skip_stack (including the
> 	   "lexing_if" override and the SKIP_NO_SKIP state).
> 
> This new comment is intended to bring a bit more clarity for any readers.
> 
> There is no intended beahvioral change to the code here. The actual code
> changes include better indentation to avoid an excessively-long line, and
> using the more descriptive INITIAL rather than 0.
> ---
>  src/glsl/glcpp/glcpp-lex.l | 45 +++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/src/glsl/glcpp/glcpp-lex.l b/src/glsl/glcpp/glcpp-lex.l
> index a029f62..5543edc 100644
> --- a/src/glsl/glcpp/glcpp-lex.l
> +++ b/src/glsl/glcpp/glcpp-lex.l
> @@ -92,14 +92,47 @@ OCTAL_INTEGER		0[0-7]*[uU]?
>  HEXADECIMAL_INTEGER	0[xX][0-9a-fA-F]+[uU]?
>  
>  %%
> -	/* Implicitly switch between SKIP and INITIAL (non-skipping);
> -	 * don't switch if some other state was explicitly set.
> +
> +	/* The handling of the SKIP vs INITIAL start states requires
> +	 * some special handling. Typically, a lexer would change
> +	 * start states with statements like "BEGIN SKIP" within the
> +	 * lexer rules. We can't get away with that here, since we
> +	 * need the parser to actually evaluate expressions for
> +	 * directives like "#if".
> +	 *
> +	 * So, here, in code that will be executed on every call to
> +	 * the lexer,and before any rules, we examine the skip_stack
> +	 * as set by the parser to know whether to change from INITIAL
> +	 * to SKIP or from SKIP back to INITIAL.
> +	 *
> +	 * Three cases cause us to switch out of the SKIP state and
> +	 * back to the INITIAL state:
> +	 *
> +	 *	1. The top of the skip_stack is of type SKIP_NO_SKIP
> +	 *	   This means we're still evaluating some #if
> +	 *	   hierarchy, but we're on a branch of it where
> +	 *	   content should not be skipped (such as "#if 1" or
> +	 *	   "#else" or so).
> +	 *
> +	 *	2. The skip_stack is NULL meaning that we've reached
> +	 *	   the last #endif.
> +	 *
> +	 *	3. The lexing_if bit is set. This indicates that we
> +	 *	   are lexing the expression following an "#if" of
> +	 *	   "#elif". Even inside an "#if 0" we need to lex this
> +	 *	   expression so the parser can correctly update the
> +	 *	   skip_stack state.
>  	 */
>  	glcpp_parser_t *parser = yyextra;
> -	if (YY_START == 0 || YY_START == SKIP) {
> -		if (parser->lexing_if || parser->skip_stack == NULL || parser->skip_stack->type == SKIP_NO_SKIP) {
> -			BEGIN 0;
> -		} else {
> +	if (YY_START == INITIAL || YY_START == SKIP) {
> +		if (parser->lexing_if ||
> +		    parser->skip_stack == NULL ||
> +		    parser->skip_stack->type == SKIP_NO_SKIP)
> +		{
> +			BEGIN INITIAL;
> +		}
> +		else
> +		{

This should used the same formatting as the rest of Mesa (the { or } on
the same line as the if or else).

>  			BEGIN SKIP;
>  		}
>  	}
>