This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.

	* 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.


	* 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.

	* 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.

	* 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.


	* 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.

	* tree-complex.c (expand_complex_division): Likewise.
No sure on this one.



	* 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.

	* 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.

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



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.

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

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

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

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.

jeff


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]