[PATCH] Prevent 'control reaches end of non-void function' warning for DO_WHILE

[Tom de Vries]

Jason,

On 18/02/12 09:33, Jason Merrill wrote:
> On 01/22/2012 03:38 AM, Tom de Vries wrote:
> 
> Sorry I didn't notice this patch until now; please CC me on C++ patches, 
> or at least mention C++ in the subject line.
> 

OK, will do.

>> +      tree expr = NULL;
>> +      append_to_statement_list (*block, &expr);
>> +      *block = expr;
> 
> Rather than doing this dance here, I think it would be better to enhance 
> append_to_statement_list to handle the case of the list argument being a 
> non-list.
> 

Added return value to append_to_statement_list, so now it's:

*block = append_to_statement_list (*block, NULL);

>> +  cp_walk_tree (&incr, cp_genericize_r, data, NULL);
>>
>> +  if (incr && EXPR_P (incr))
>> +    SET_EXPR_LOCATION (incr, start_locus);
> 
> It might be better to set the location on incr before genericizing, so 
> that the location can trickle down.
> 

I see. I reordered the code.

>> +         t = build1 (GOTO_EXPR, void_type_node, get_bc_label (bc_break));
>> +         SET_EXPR_LOCATION (t, start_locus);
> 
> Here you can use build1_loc instead of two separate statements.
> 

Done.

>> -      /* If we use a LOOP_EXPR here, we have to feed the whole thing
>> -        back through the main gimplifier to lower it.  Given that we
>> -        have to gimplify the loop body NOW so that we can resolve
>> -        break/continue stmts, seems easier to just expand to gotos.  */
> 
> Since we're now lowering the loop at genericize time rather than 
> gimplify, what's the rationale for not using LOOP_EXPR/EXIT_EXPR?  We 
> should still say something here.  I suppose the rationale is that the C 
> front end currently goes straight to gotos.
> 

I think we could indeed use LOOP_EXPR/EXIT_EXPR. I'm not sure what the gain
would be, since at least for C it would be a construct alive only between
genericize and gimplify. But I guess it makes sense from orthogonality point of
view.

Added a comment with todo.

>>           if (cond != error_mark_node)
>>             {
>> -             gimplify_expr (&cond, &exit_seq, NULL, is_gimple_val, fb_rvalue);
>> -             stmt = gimple_build_cond (NE_EXPR, cond,
>> -                                       build_int_cst (TREE_TYPE (cond), 0),
>> -                                       gimple_label_label (top),
>> -                                       get_bc_label (bc_break));
>> -             gimple_seq_add_stmt (&exit_seq, stmt);
>> +             cond = build2 (NE_EXPR, boolean_type_node, cond,
>> +                            build_int_cst (TREE_TYPE (cond), 0));
> 
> I don't think we still need this extra comparison to 0, that seems like 
> a gimple-specific thing.
> 

OK, removed.

>>    if (FOR_INIT_STMT (stmt))
>> +    append_to_statement_list (FOR_INIT_STMT (stmt), &expr);
>>
>> +  genericize_cp_loop (&loop, EXPR_LOCATION (stmt), FOR_COND (stmt),
>> +                     FOR_BODY (stmt), FOR_EXPR (stmt), 1, walk_subtrees, data);
> 
> The call to genericize_cp_loop will clear *walk_subtrees, which means we 
> don't genericize the FOR_INIT_STMT.
> 

I see. Added call to cp_generize_r.

>> +  tree jump = build_and_jump (&label);
> 
> Again, let's use build1_loc.
> 

Done.

>> +  *stmt_p = build_and_jump (&label);
>> +  SET_EXPR_LOCATION (*stmt_p, location);
> 
> And here.
> 

Done.

>> +  stmt = make_node (OMP_FOR);
> 
> Why make a new OMP_FOR rather than repurpose the one we already have? 
> We've already modified its operands.
> 

Done.

Bootstrapped and reg-tested on x86_64.

OK for trunk?

Thanks,
- Tom

> Jason

2012-04-14  Tom de Vries  <tom@codesourcery.com>

	* tree-iterator.c (append_to_statement_list_1, append_to_statement_list)
	(append_to_statement_list_force): Return resulting list.  Handle
	list_p == NULL.
	* tree-iterator.h (append_to_statement_list)
	(append_to_statement_list_force): Add tree return type.

	* cp-gimplify.c (begin_bc_block): Add location parameter and use as
	location argument to create_artificial_label.
	(finish_bc_block): Change return type to void.  Remove body_seq
	parameter, and add block parameter.  Append label to STMT_LIST and
	return in block.
	(gimplify_cp_loop, gimplify_for_stmt, gimplify_while_stmt)
	(gimplify_do_stmt, gimplify_switch_stmt): Remove function.
	(genericize_cp_loop, genericize_for_stmt, genericize_while_stmt)
	(genericize_do_stmt, genericize_switch_stmt, genericize_continue_stmt)
	(genericize_break_stmt, genericize_omp_for_stmt): New function.
	(cp_gimplify_omp_for): Remove bc_continue processing.
	(cp_gimplify_expr): Genericize VEC_INIT_EXPR.
	(cp_gimplify_expr): Mark FOR_STMT, WHILE_STMT, DO_STMT, SWITCH_STMT,
	CONTINUE_STMT, and BREAK_STMT as unreachable.
	(cp_genericize_r): Genericize FOR_STMT, WHILE_STMT, DO_STMT,
	SWITCH_STMT, CONTINUE_STMT, BREAK_STMT and OMP_FOR.
	(cp_genericize_tree): New function, factored out of ...
	(cp_genericize): ... this function.

	* g++.dg/pr51264-4.C: New test.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr51264.8.patch
Type: text/x-patch
Size: 19910 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20120414/0869849e/attachment.bin>