[ruby-core:25725] Re: turning off indentation warnings

On Sep 10, 2009, at 06:39 , Nobuyoshi Nakada wrote:

> Hi,
>
> At Mon, 7 Sep 2009 07:53:27 +0900,
> Aaron Patterson wrote in [ruby-core:25442]:
>> Is there a way in 1.9 to turn off only indentation warnings?  I like
>> to keep warnings turned on, but indentation warnings don't make sense
>> when the output is generated, like with racc.
>
> Currently, nothing.  How about magic comments?

I like this. It allows us to generate code that can bypass these  
warnings w/o too much extra work.

I've looked into adding a global variable. I think it looks better,  
but it requires modifying the parser to evaluate SOME expressions at  
parse time and I think that is a can of worms we don't want to open.

I do have some feedback inline below:

> Index: parse.y
> ===================================================================
> --- parse.y	(revision 24824)
> +++ parse.y	(working copy)
> @@ -244,4 +244,5 @@ struct parser_params {
>
>     token_info *parser_token_info;
> +    int parser_token_info_enabled;
> #else
>     /* Ripper only */
> @@ -587,4 +588,7 @@ static void ripper_compile_error(struct
> static void token_info_push(struct parser_params*, const char *token);
> static void token_info_pop(struct parser_params*, const char *token);
> +#else
> +#define token_info_push(parser, token) /* nothing */
> +#define token_info_pop(parser, token) /* nothing */
> #endif
> %}
> @@ -2970,7 +2974,5 @@ primary_value	: primary
> k_begin		: keyword_begin
> 		    {
> -#ifndef RIPPER
> -			if (RTEST(ruby_verbose)) token_info_push(parser, "begin");
> -#endif
> +			token_info_push(parser, "begin");
> 		    }
> 		;
> @@ -2978,7 +2980,5 @@ k_begin		: keyword_begin
> k_if		: keyword_if
> 		    {
> -#ifndef RIPPER
> -			if (RTEST(ruby_verbose)) token_info_push(parser, "if");
> -#endif
> +			token_info_push(parser, "if");
> 		    }
> 		;
> @@ -2986,7 +2986,5 @@ k_if		: keyword_if
> k_unless	: keyword_unless
> 		    {
> -#ifndef RIPPER
> -			if (RTEST(ruby_verbose)) token_info_push(parser, "unless");
> -#endif
> +			token_info_push(parser, "unless");
> 		    }
> 		;
> @@ -2994,7 +2992,5 @@ k_unless	: keyword_unless
> k_while		: keyword_while
> 		    {
> -#ifndef RIPPER
> -			if (RTEST(ruby_verbose)) token_info_push(parser, "while");
> -#endif
> +			token_info_push(parser, "while");
> 		    }
> 		;
> @@ -3002,7 +2998,5 @@ k_while		: keyword_while
> k_until		: keyword_until
> 		    {
> -#ifndef RIPPER
> -			if (RTEST(ruby_verbose)) token_info_push(parser, "until");
> -#endif
> +			token_info_push(parser, "until");
> 		    }
> 		;
> @@ -3010,7 +3004,5 @@ k_until		: keyword_until
> k_case		: keyword_case
> 		    {
> -#ifndef RIPPER
> -			if (RTEST(ruby_verbose)) token_info_push(parser, "case");
> -#endif
> +			token_info_push(parser, "case");
> 		    }
> 		;
> @@ -3018,7 +3010,5 @@ k_case		: keyword_case
> k_for		: keyword_for
> 		    {
> -#ifndef RIPPER
> -			if (RTEST(ruby_verbose)) token_info_push(parser, "for");
> -#endif
> +			token_info_push(parser, "for");
> 		    }
> 		;
> @@ -3026,7 +3016,5 @@ k_for		: keyword_for
> k_class		: keyword_class
> 		    {
> -#ifndef RIPPER
> -			if (RTEST(ruby_verbose)) token_info_push(parser, "class");
> -#endif
> +			token_info_push(parser, "class");
> 		    }
> 		;
> @@ -3034,7 +3022,5 @@ k_class		: keyword_class
> k_module	: keyword_module
> 		    {
> -#ifndef RIPPER
> -			if (RTEST(ruby_verbose)) token_info_push(parser, "module");
> -#endif
> +			token_info_push(parser, "module");
> 		    }
> 		;
> @@ -3042,7 +3028,5 @@ k_module	: keyword_module
> k_def		: keyword_def
> 		    {
> -#ifndef RIPPER
> -			if (RTEST(ruby_verbose)) token_info_push(parser, "def");
> -#endif
> +			token_info_push(parser, "def");
> 		    }
> 		;
> @@ -3050,7 +3034,5 @@ k_def		: keyword_def
> k_end		: keyword_end
> 		    {
> -#ifndef RIPPER
> -			if (RTEST(ruby_verbose)) token_info_pop(parser, "end");  /* POP */
> -#endif
> +			token_info_pop(parser, "end");  /* POP */
> 		    }
> 		;

I like this a lot. It makes it a lot more readable.

That last comment is completely unnecessary.

> @@ -4826,5 +4808,5 @@ token_info_push(struct parser_params *pa
>     token_info *ptinfo;
>
> -    if (compile_for_eval) return;
> +    if (!parser->parser_token_info_enabled) return;
>     ptinfo = ALLOC(token_info);
>     ptinfo->token = token;
> @@ -4855,7 +4837,9 @@ token_info_pop(struct parser_params *par
> 	goto finish;
>     }
> -    rb_compile_warning(ruby_sourcefile, linenum,
> -               "mismatched indentations at '%s' with '%s' at %d",
> -	       token, ptinfo->token, ptinfo->linenum);
> +    if (parser->parser_token_info_enabled) {
> +	rb_compile_warn(ruby_sourcefile, linenum,
> +			"mismatched indentations at '%s' with '%s' at %d",
> +			token, ptinfo->token, ptinfo->linenum);
> +    }
>
>   finish:
> @@ -4998,4 +4982,7 @@ yycompile0(VALUE arg, int tracing)
>     parser_prepare(parser);
>     deferred_nodes = 0;
> +#ifndef RIPPER
> +    parser->parser_token_info_enabled = !compile_for_eval && RTEST 
> (ruby_verbose);
> +#endif
>     n = yyparse((void*)parser);
>     ruby_debug_lines = 0;
> @@ -6123,4 +6110,28 @@ magic_comment_encoding(struct parser_par
> }
>
> +static void
> +set_boolean(struct parser_params *parser, int *p, const char *val)
> +{
> +    if (strcasecmp(val, "enable") == 0 ||
> +	strcasecmp(val, "yes") == 0 ||
> +	strcasecmp(val, "true") == 0) {
> +	*p = 1;
> +    }
> +    else if (strcasecmp(val, "disable") == 0 ||
> +	strcasecmp(val, "no") == 0 ||
> +	strcasecmp(val, "false") == 0) {
> +	*p = 0;
> +    }
> +    else {
> +	rb_compile_warning(ruby_sourcefile, ruby_sourceline, "invalid  
> boolean %s", val);
> +    }
> +}

Is this really necessary? I'd rather stick to ruby values and remove  
enable/yes/disable/no.

Also, it is only being used once, so why bother abstracting it? Matz  
is (rightfully) against using magic comments, so why enable them  
further?

> +
> +static void
> +parser_set_token_info(struct parser_params *parser, const char  
> *name, const char *val)
> +{
> +    set_boolean(parser, &parser->parser_token_info_enabled, val);
> +}
> +
> struct magic_comment {
>     const char *name;
> @@ -6132,4 +6143,5 @@ static const struct magic_comment magic_
>     {"coding", magic_comment_encoding, parser_encode_length},
>     {"encoding", magic_comment_encoding, parser_encode_length},
> +    {"warn-indent", parser_set_token_info},
> };
> #endif
> @@ -6178,9 +6190,15 @@ parser_magic_comment(struct parser_param
> 	: ((_s) = STR_NEW((_p), (_n))))
>
> -    if (len <= 7) return Qfalse;
> -    if (!(beg = magic_comment_marker(str, len))) return Qfalse;
> -    if (!(end = magic_comment_marker(beg, str + len - beg))) return  
> Qfalse;
> -    str = beg;
> -    len = end - beg - 3;
> +    if (*str == '%') {
> +	str++;
> +	len--;
> +    }
> +    else {
> +	if (len <= 7) return Qfalse;
> +	if (!(beg = magic_comment_marker(str, len))) return Qfalse;
> +	if (!(end = magic_comment_marker(beg, str + len - beg))) return  
> Qfalse;
> +	str = beg;
> +	len = end - beg - 3;
> +    }

What is this change? I don't get the '%' part. It could use a comment  
for clarification because it isn't obvious in this area.