PATCH for c++/70639 (ICE-on-valid with -Wmisleading-indentation and switch)

This is an ICE-on-valid-though-weirdo (TM) problem.  We were trying to warn
about misleading indentation for a switch statement, but guard_tinfo_to_string
doesn't know what to do with RID_SWITCH and so a crash ensues.  Rather than
teaching it about RID_SWITCH I think this warning can't usefully warn about
switch statements at all, similarly to do-while constructs.

David, what do you think?

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-04-13  Marek Polacek  <polacek@redhat.com>

	PR c++/70639
	* c-indentation.c (should_warn_for_misleading_indentation): Bail out
	for switch statements, too.

	* c-c++-common/Wmisleading-indentation-4.c: New test.


	Marek

On Wed, 2016-04-13 at 17:31 +0200, Marek Polacek wrote:
> This is an ICE-on-valid-though-weirdo (TM) problem.  We were trying
> to warn
> about misleading indentation for a switch statement, but
> guard_tinfo_to_string
> doesn't know what to do with RID_SWITCH and so a crash ensues. 
>  Rather than
> teaching it about RID_SWITCH I think this warning can't usefully warn
> about
> switch statements at all, similarly to do-while constructs.
> 
> David, what do you think?

My first thought was that warn_for_misleading_indentation is only meant
to be called by the frontend on if/else/while/for constructs, and I
wondered how it was getting called on a "switch" statement.  Answering
my own question, I see that cp_parser_selection_statement handles both
"if" and "switch" in the C++ FE, and builds the guard_tinfo, and passes
it to cp_parser_implicitly_scoped_statement.  Looks like I missed this
possibility.

So another way to fix this would be to update
 cp_parser_implicitly_scoped_statement to wrap this:

  token_indent_info next_tinfo
    = get_token_indent_info (cp_lexer_peek_token (parser->lexer));
  warn_for_misleading_indentation (guard_tinfo, body_tinfo, next_tinfo);

so it only happens if we have an "if" token, not for a "switch".

Your approach encapsulates the logic for rejecting this situation
within should_warn_for_misleading_indentation, rather than at the
callers, which is arguably better for modularity (similar to how we
already call it for "do", which is then always rejected).

So your patch looks good to me (I don't have formal approval rights for
this though).

> Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> 2016-04-13  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c++/70639
> 	* c-indentation.c (should_warn_for_misleading_indentation):
> Bail out
> 	for switch statements, too.
> 
> 	* c-c++-common/Wmisleading-indentation-4.c: New test.
> 
> diff --git gcc/c-family/c-indentation.c gcc/c-family/c-indentation.c
> index 1da3f68..8c33686 100644
> --- gcc/c-family/c-indentation.c
> +++ gcc/c-family/c-indentation.c
> @@ -239,10 +239,11 @@ should_warn_for_misleading_indentation (const
> token_indent_info &guard_tinfo,
>    if (line_table->seen_line_directive)
>      return false;
>  
> -  /* We can't usefully warn about do-while statements since the
> bodies of these
> -     statements are always explicitly delimited at both ends, so
> control flow is
> -     quite obvious.  */
> -  if (guard_tinfo.keyword == RID_DO)
> +  /* We can't usefully warn about do-while and switch statements
> since the
> +     bodies of these statements are always explicitly delimited at
> both ends,
> +     so control flow is quite obvious.  */
> +  if (guard_tinfo.keyword == RID_DO
> +      || guard_tinfo.keyword == RID_SWITCH)
>      return false;
>  
>    /* If the token following the body is a close brace or an "else"
> diff --git gcc/testsuite/c-c++-common/Wmisleading-indentation-4.c
> gcc/testsuite/c-c++-common/Wmisleading-indentation-4.c
> index e69de29..d15a479 100644
> --- gcc/testsuite/c-c++-common/Wmisleading-indentation-4.c
> +++ gcc/testsuite/c-c++-common/Wmisleading-indentation-4.c
> @@ -0,0 +1,11 @@
> +/* PR c++/70639 */
> +/* { dg-do compile } */
> +/* { dg-options "-Wmisleading-indentation" } */
> +
> +void bar (int);
> +void
> +foo (int x)
> +{
> +  switch (x);
> +	bar (x);
> +}
> 
> 	Marek

On 04/13/2016 06:11 PM, David Malcolm wrote:
> Your approach encapsulates the logic for rejecting this situation
> within should_warn_for_misleading_indentation, rather than at the
> callers, which is arguably better for modularity (similar to how we
> already call it for "do", which is then always rejected).
>
> So your patch looks good to me (I don't have formal approval rights for
> this though).

I'll ack it, I also thought it looked reasonable.


Bernd