Re: Implement -Wimplicit-fallthrough (take 2): questionable code

[Marek Polacek <polacek at redhat dot com>]

On Wed, Aug 03, 2016 at 10:56:08AM -0600, Jeff Law wrote:
> On 07/27/2016 10:53 AM, Marek Polacek wrote:
> > These are the cases where I wasn't sure if the falls through were intentional
> > or not.
> > 
> > This patch has been tested on powerpc64le-unknown-linux-gnu, aarch64-linux-gnu,
> > and x86_64-redhat-linux.
> > 
> > 2016-07-27  Marek Polacek  <polacek@redhat.com>
> > 
> > 	PR c/7652
> > gcc/
> > 	* config/i386/i386.c (ix86_expand_branch): Add gcc_fallthrough..
> I'm pretty sure this is an intended fallthru.

Ok.
 
> > 	* cselib.c (cselib_expand_value_rtx_1): Likewise.
> I'm pretty sure this is an intended fallthru.  But rather than fallthru, can
> we just "return orig;"?  If more code gets added here it'll be less and less
> clear if we continue to fall thru.

Done in the patch I just posted.
 
> > 	* gensupport.c (get_alternatives_number): Likewise.
> > 	(subst_dup): Likewise.
> These are definitely an intended fallthrough.  E & V are essentially the
> same thing, except that for V the vector may be NULL.

Ok.
 
> > 	* gimplify.c (goa_stabilize_expr): Likewise.
> Definitely intended fallthrough.  We're "cleverly" handling the 2nd operand
> of binary expressions, then falling into the unary expression cases which
> handle the 1st operand.

Ok.
 
> > 	* hsa-gen.c (gen_hsa_insn_for_internal_fn_call): Likewise.
> I think this is a bug, but please contact Martin Jambor directly on this to
> confirm.

Done.

> > 	* reg-stack.c (get_true_reg): Likewise.
> Pretty sure this is intentional.  Like the cselib.c case, I'd prefer to just
> duplicate the code from teh fallthru path since it's trivial and makes the
> intent clearer.

Done.

> > 	* tree-complex.c (expand_complex_division): Likewise.
> No sure on this one.
 
Ok, I kept it as it was.
 
> > 	* tree-data-ref.c (get_references_in_stmt): Likewise.
> Intentional fallthru.  See how we change ref.is_read for IFN_MASK_LOAD, then
> test it in the fallthru path.

Ok. 

> > 	* tree-pretty-print.c (dump_generic_node): Likewise.
> ?!?  This looks like a false positive from your warning.  We're not falling
> into any case statements here AFAICT.

I've fixed the warning to not warn here anymore.

> > 	* var-tracking.c (adjust_mems): Likewise.
> Definitely an intended fallthru.   Though I don't like the way this code is
> structured at all.

Yea, it's ugly.

> > gcc/cp/
> > 	* call.c (add_builtin_candidate): Add gcc_fallthrough.
> > 	* cxx-pretty-print.c (pp_cxx_unqualified_id): Likewise.
> > 	* parser.c (cp_parser_skip_to_end_of_statement): Likewise.
> > 	(cp_parser_cache_defarg): Likewise.
> No idea on these.
> 
> 
> > gcc/c-family/
> > 	* c-ada-spec.c (dump_generic_ada_node): Add gcc_fallthrough.
> I don't think this is supposed to fall thru.  If it falls through then it's
> going to issue an error about an unexpected TREE_VEC node when we were
> actually working on a TREE_BINFO node.   I think a return 0 is appropriate
> here.
> 
> 
> > libcpp/
> > 	* pch.c (write_macdef): Add CPP_FALLTHRU.
> > 	* lex.c (_cpp_lex_direct): Likewise.
> No idea on these.
> 
> Sooo.  For those where I've indicated fallthru is intentional, drop the
> comment and/or rewrite to avoid fallthru as indicated.
 
Done.

> You've got one that looks like a error in your warning
> (tree-pretty-print.c).
 
Fixed.

> Sync with Martin J. on the hsa warning.  I think it's a bug in the hsa code,
> but confirm with Martin.
 
Done.

> I think the c-ada-spec should be twiddled to avoid the fallthru with a
> simple "return 0;"
 
Done.

> For the rest, use your patch as-is.  That preserves behavior and indicates
> we're not sure if fallthru is appropriate or not.  For cp/ feel free to ping
> Jason directly on those for his thoughts.

Thanks a lot for reviewing this.  I'll post an updated version later on.

	Marek