Re: PATCH: diagnostic: fix PR c++/19756

["Manuel LÃpez-IbÃÃez" <lopezibanez at gmail dot com>]

On 01/12/06, Dirk Mueller <dmuell@gmx.net> wrote:
> Hi,

Hi. First, I should mention I cannot approve your patch. Actually I am
a newbie here.

> I don't quite understand why the implementation in the C frontend is so overly
> complicated, while the simple patch (see below) works as well.

Overly complicated? Let's see the code in c-typeck.c (c_finish_if_stmt)

 /* Diagnose an ambiguous else if if-then-else is nested inside if-then.  */
 if (warn_parentheses && nested_if && else_block == NULL)
   {
     tree inner_if = then_block;

     /* We know from the grammar productions that there is an IF nested
        within THEN_BLOCK.  Due to labels and c99 conditional declarations,
        it might not be exactly THEN_BLOCK, but should be the last
        non-container statement within.  */
     while (1)
       switch (TREE_CODE (inner_if))
         {
         case COND_EXPR:
           goto found;
         case BIND_EXPR:
           inner_if = BIND_EXPR_BODY (inner_if);
           break;
         case STATEMENT_LIST:
           inner_if = expr_last (then_block);
           break;
         case TRY_FINALLY_EXPR:
         case TRY_CATCH_EXPR:
           inner_if = TREE_OPERAND (inner_if, 0);
           break;
         default:
           gcc_unreachable ();
         }
   found:

     if (COND_EXPR_ELSE (inner_if))
        warning (OPT_Wparentheses,
                 "%Hsuggest explicit braces to avoid ambiguous %<else%>",
                 &if_locus);
   }

It seems to me that this code is handling some C99 constructions that
are not handled in your patch (presumably because they don't exist in
C++).

Other than that, I think the differences are pretty minor. For
example. COND_EXPR_ELSE is practically the same as ELSE_CLAUSE.
Perhaps, it could be advantageous to unify the code and move it to
c-common.c


>               }
> +           else
> +           {
> +             tree inner_then = THEN_CLAUSE (statement);
> +

I think you can move this initialisation...

> +             if (warn_parentheses && no_then_braces)
> +               {

... here and actually remove one indentation level.

> +                 if (TREE_CODE (inner_then) == STATEMENT_LIST
> +                     && STATEMENT_LIST_TAIL (inner_then))
> +                   inner_then = STATEMENT_LIST_TAIL (inner_then)->stmt;

I guess there is no practical difference but, why not use
inner_then = expr_last (inner_then)
here? The latter seems more abstract to me. (Actually, just a few
lines above the definition of STATEMENT_LIST_TAIL it says "Use the
interface in tree-iterator.h to access this node").

Cheers,

Manuel.