diff mbox

Implement -Wimplicit-fallthrough (version 9)

Message ID 3386107.i6u4Hqc2M6@polaris
State New
Headers show

Commit Message

Eric Botcazou Oct. 8, 2016, 5:04 p.m. UTC 
> testing completed successfully, so I've installed the patch with this
> ChangeLog entry:
> 
> 2016-09-26  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
> 
> 	gcc:
> 	* config/i386/i386.c (ix86_print_operand)
> 	[HAVE_AS_IX86_CMOV_SUN_SYNTAX]: Add gcc_fallthrough.
> 	* config/sparc/sparc.c (check_pic): Add fallthrough comment.
> 	(epilogue_renumber): Likewise.
> 
> 	gcc/ada:
> 	* gcc-interface/decl.c: Fix fall through comment formatting.
> 	* gcc-interface/misc.c: Likewise.
> 	* gcc-interface/trans.c: Likewise.
> 	* gcc-interface/utils.c: Likewise.
> 	* gcc-interface/utils2.c: Likewise.

This is a revealing example of how excessive pickiness in warnings can be 
counter-productive: after Jakub's latest patches (thanks!) accepting the 
original formatting of gcc-interface, I reverted part #2 of the above patch... 
only to discover that bootstrap was still broken because of a -Wimplicit-
fallthrough warning, but this time for a missing break:


So the issue went unnoticed among the slew of false positives the first time 
and a genuine error was overlooked...

Tested on x86_64-suse-linux, applied on the mainline.


2016-10-08  Eric Botcazou  <ebotcazou@adacore.com>

	* gcc-interface/utils.c (convert) <VECTOR_CST>: Add missing break.

	Revert
	2016-09-26  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	* gcc-interface/decl.c: Fix fall through comment formatting.
	* gcc-interface/misc.c: Likewise.
	* gcc-interface/trans.c: Likewise.
	* gcc-interface/utils.c: Likewise.
	* gcc-interface/utils2.c: Likewise.

Comments

Marek Polacek Oct. 9, 2016, 9:19 a.m. UTC | #1 
On Sat, Oct 08, 2016 at 07:04:41PM +0200, Eric Botcazou wrote:
> > testing completed successfully, so I've installed the patch with this
> > ChangeLog entry:
> > 
> > 2016-09-26  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
> > 
> > 	gcc:
> > 	* config/i386/i386.c (ix86_print_operand)
> > 	[HAVE_AS_IX86_CMOV_SUN_SYNTAX]: Add gcc_fallthrough.
> > 	* config/sparc/sparc.c (check_pic): Add fallthrough comment.
> > 	(epilogue_renumber): Likewise.
> > 
> > 	gcc/ada:
> > 	* gcc-interface/decl.c: Fix fall through comment formatting.
> > 	* gcc-interface/misc.c: Likewise.
> > 	* gcc-interface/trans.c: Likewise.
> > 	* gcc-interface/utils.c: Likewise.
> > 	* gcc-interface/utils2.c: Likewise.
> 
> This is a revealing example of how excessive pickiness in warnings can be 
> counter-productive: after Jakub's latest patches (thanks!) accepting the 
> original formatting of gcc-interface, I reverted part #2 of the above patch... 
> only to discover that bootstrap was still broken because of a -Wimplicit-
> fallthrough warning, but this time for a missing break:

I really really don't see why anyone would think that those '...' bring
any additional information.  Since Rainer has changed this, I see zero
point in changing it back.

> Index: gcc-interface/utils.c
> ===================================================================
> --- gcc-interface/utils.c	(revision 324591)
> +++ gcc-interface/utils.c	(working copy)
> @@ -4289,6 +4289,7 @@ convert (tree type, tree expr)
>  	  TREE_TYPE (expr) = type;
>  	  return expr;
>  	}
> +      break;
>  
>      case CONSTRUCTOR:
>        /* If we are converting a CONSTRUCTOR to a mere type variant, or to
> 
> So the issue went unnoticed among the slew of false positives the first time 
> and a genuine error was overlooked...

It wasn't overlooked, there was a bug that I've fixed already which caused
missing warnings.

	Marek
Eric Botcazou Oct. 9, 2016, 9:43 a.m. UTC | #2 
> I really really don't see why anyone would think that those '...' bring
> any additional information.  Since Rainer has changed this, I see zero
> point in changing it back.

Yet doing it revealed an oversight in the first patch...

> It wasn't overlooked, there was a bug that I've fixed already which caused
> missing warnings.

Nope, see this hunk in the first patch:

@@ -4271,6 +4271,8 @@ convert (tree type, tree expr)
          return expr;
        }
 
+      /* fall through */
+
     case CONSTRUCTOR:
       /* If we are converting a CONSTRUCTOR to a mere type variant, or to
         another padding type around the same type, just make a new one in

It's plain wrong, it should have been a break instead, but this was overlooked 
by everyone because of the bunch of false positives.
diff mbox

Patch

Index: gcc-interface/decl.c
===================================================================
--- gcc-interface/decl.c	(revision 240888)
+++ gcc-interface/decl.c	(working copy)
@@ -596,7 +596,7 @@  gnat_to_gnu_entity (Entity_Id gnat_entit
 	gnu_expr
 	  = gnat_to_gnu_external (Expression (Declaration_Node (gnat_entity)));
 
-      /* fall through */
+      /* ... fall through ... */
 
     case E_Exception:
     case E_Loop_Parameter:
@@ -3369,7 +3369,7 @@  gnat_to_gnu_entity (Entity_Id gnat_entit
 	  break;
 	}
 
-      /* fall through */
+      /* ... fall through ... */
 
     case E_Record_Subtype:
       /* If Cloned_Subtype is Present it means this record subtype has
@@ -3804,7 +3804,7 @@  gnat_to_gnu_entity (Entity_Id gnat_entit
 	    break;
 	}
 
-      /* fall through */
+      /* ... fall through ... */
 
     case E_Allocator_Type:
     case E_Access_Type:
@@ -6882,7 +6882,7 @@  choices_to_gnu (tree operand, Node_Id ch
 	      break;
 	    }
 
-	  /* fall through */
+	  /* ... fall through ... */
 
 	case N_Character_Literal:
 	case N_Integer_Literal:
@@ -8089,7 +8089,7 @@  annotate_value (tree gnu_size)
       else
 	return Uint_Minus_1;
 
-      /* fall through */
+      /* Fall through... */
 
     default:
       return No_Uint;
Index: gcc-interface/misc.c
===================================================================
--- gcc-interface/misc.c	(revision 240888)
+++ gcc-interface/misc.c	(working copy)
@@ -157,7 +157,7 @@  gnat_handle_option (size_t scode, const
     case OPT_gant:
       warning (0, "%<-gnat%> misspelled as %<-gant%>");
 
-      /* fall through */
+      /* ... fall through ... */
 
     case OPT_gnat:
     case OPT_gnatO:
@@ -485,13 +485,13 @@  gnat_print_type (FILE *file, tree node,
       else
 	print_node (file, "index type", TYPE_INDEX_TYPE (node), indent + 4);
 
-      /* fall through */
+      /* ... fall through ... */
 
     case ENUMERAL_TYPE:
     case BOOLEAN_TYPE:
       print_node_brief (file, "RM size", TYPE_RM_SIZE (node), indent + 4);
 
-      /* fall through */
+      /* ... fall through ... */
 
     case REAL_TYPE:
       print_node_brief (file, "RM min", TYPE_RM_MIN_VALUE (node), indent + 4);
Index: gcc-interface/trans.c
===================================================================
--- gcc-interface/trans.c	(revision 240888)
+++ gcc-interface/trans.c	(working copy)
@@ -844,7 +844,7 @@  lvalue_required_p (Node_Id gnat_node, tr
 		 && Ekind (Entity (gnat_temp)) == E_Enumeration_Literal))
 	  return 1;
 
-      /* fall through */
+      /* ... fall through ... */
 
     case N_Slice:
       /* Only the array expression can require an lvalue.  */
@@ -890,7 +890,7 @@  lvalue_required_p (Node_Id gnat_node, tr
 	if (!constant)
 	  return 1;
 
-      /* fall through */
+      /* ... fall through ... */
 
     case N_Type_Conversion:
     case N_Qualified_Expression:
@@ -914,7 +914,7 @@  lvalue_required_p (Node_Id gnat_node, tr
 				  get_unpadded_type (Etype (gnat_parent)),
 				  true, false, true);
 
-      /* fall through */
+      /* ... fall through ... */
 
     default:
       return 0;
@@ -1681,7 +1681,7 @@  Attribute_to_gnu (Node_Id gnat_node, tre
 	  break;
 	}
 
-      /* fall through */
+      /* ... fall through ... */
 
     case Attr_Access:
     case Attr_Unchecked_Access:
@@ -1938,7 +1938,7 @@  Attribute_to_gnu (Node_Id gnat_node, tre
 	  break;
 	}
 
-      /* fall through */
+      /* ... fall through ... */
 
     case Attr_Length:
       {
@@ -2393,7 +2393,7 @@  Attribute_to_gnu (Node_Id gnat_node, tre
       /* We treat Model as identical to Machine.  This is true for at least
 	 IEEE and some other nice floating-point systems.  */
 
-      /* fall through */
+      /* ... fall through ... */
 
     case Attr_Machine:
       /* The trick is to force the compiler to store the result in memory so
@@ -2537,7 +2537,7 @@  Case_Statement_to_gnu (Node_Id gnat_node
 		  break;
 		}
 
-	      /* fall through */
+	      /* ... fall through ... */
 
 	    case N_Character_Literal:
 	    case N_Integer_Literal:
@@ -4007,7 +4007,7 @@  node_is_atomic (Node_Id gnat_node)
 	  && Has_Atomic_Components (Entity (Prefix (gnat_node))))
 	return true;
 
-      /* fall through */
+      /* ... fall through ... */
 
     case N_Explicit_Dereference:
       return Is_Atomic (Etype (gnat_node));
@@ -4123,7 +4123,7 @@  atomic_access_required_p (Node_Id gnat_n
       /* Nothing to do if we are the prefix of an attribute, since we do not
 	 want an atomic access for things like 'Size.  */
 
-      /* fall through */
+      /* ... fall through ... */
 
     case N_Reference:
       /* The N_Reference node is like an attribute.  */
@@ -6580,7 +6580,7 @@  gnat_to_gnu (Node_Id gnat_node)
 	  break;
 	}
 
-      /* fall through */
+      /* ... fall through ... */
 
     case N_Op_Eq:
     case N_Op_Ne:
@@ -6747,7 +6747,7 @@  gnat_to_gnu (Node_Id gnat_node)
 	  break;
 	}
 
-      /* fall through */
+      /* ... fall through ... */
 
     case N_Op_Minus:
     case N_Op_Abs:
@@ -8344,7 +8344,7 @@  gnat_gimplify_expr (tree *expr_p, gimple
 	    break;
 	  }
 
-      /* fall through */
+      /* ... fall through ... */
 
     default:
       return GS_UNHANDLED;
@@ -9867,7 +9867,7 @@  set_gnu_expr_location_from_node (tree no
       if (EXPR_P (TREE_OPERAND (node, 1)))
 	set_gnu_expr_location_from_node (TREE_OPERAND (node, 1), gnat_node);
 
-      /* fall through */
+      /* ... fall through ... */
 
     default:
       if (!REFERENCE_CLASS_P (node) && !EXPR_HAS_LOCATION (node))
Index: gcc-interface/utils.c
===================================================================
--- gcc-interface/utils.c	(revision 240888)
+++ gcc-interface/utils.c	(working copy)
@@ -3166,7 +3166,7 @@  create_subprog_decl (tree name, tree asm
 				    NULL_TREE, NULL_TREE),
 			 ATTR_FLAG_TYPE_IN_PLACE);
 
-      /* fall through */
+      /* ... fall through ... */
 
     case is_enabled:
       DECL_DECLARED_INLINE_P (subprog_decl) = 1;
@@ -4270,8 +4270,7 @@  convert (tree type, tree expr)
 	  TREE_TYPE (expr) = type;
 	  return expr;
 	}
-
-      /* fall through */
+      break;
 
     case CONSTRUCTOR:
       /* If we are converting a CONSTRUCTOR to a mere type variant, or to
@@ -4510,7 +4509,7 @@  convert (tree type, tree expr)
 					  convert (TREE_TYPE (type),
 						   TYPE_MIN_VALUE (type))));
 
-      /* fall through */
+      /* ... fall through ... */
 
     case ENUMERAL_TYPE:
     case BOOLEAN_TYPE:
@@ -4587,7 +4586,7 @@  convert (tree type, tree expr)
 	  return gnat_build_constructor (type, v);
 	}
 
-      /* fall through */
+      /* ... fall through ... */
 
     case ARRAY_TYPE:
       /* In these cases, assume the front-end has validated the conversion.
@@ -4703,7 +4702,7 @@  convert_to_index_type (tree expr)
 	  break;
       }
 
-      /* fall through */
+      /* ... fall through ... */
 
     case NON_LVALUE_EXPR:
       return fold_build1 (code, sizetype,
Index: gcc-interface/utils2.c
===================================================================
--- gcc-interface/utils2.c	(revision 240888)
+++ gcc-interface/utils2.c	(working copy)
@@ -180,7 +180,7 @@  known_alignment (tree exp)
 	  return known_alignment (t);
       }
 
-      /* fall through */
+      /* ... fall through ... */
 
     default:
       /* For other pointer expressions, we assume that the pointed-to object
@@ -1011,7 +1011,7 @@  build_binary_op (enum tree_code op_code,
       if (!operation_type)
 	operation_type = TREE_TYPE (left_type);
 
-      /* fall through */
+      /* ... fall through ... */
 
     case ARRAY_RANGE_REF:
       /* First look through conversion between type variants.  Note that
@@ -1230,7 +1230,7 @@  build_binary_op (enum tree_code op_code,
 	op_code = MINUS_EXPR;
       modulus = NULL_TREE;
 
-      /* fall through */
+      /* ... fall through ... */
 
     case PLUS_EXPR:
     case MINUS_EXPR:
@@ -1244,7 +1244,7 @@  build_binary_op (enum tree_code op_code,
 	  = gnat_type_for_mode (TYPE_MODE (operation_type),
 				TYPE_UNSIGNED (operation_type));
 
-      /* fall through */
+      /* ... fall through ... */
 
     default:
     common:
@@ -1466,7 +1466,7 @@  build_unary_op (enum tree_code op_code,
 	    return build_unary_op (ADDR_EXPR, result_type,
 				   TREE_OPERAND (operand, 0));
 
-	  /* fallthru */
+	  /* ... fallthru ... */
 
 	case VIEW_CONVERT_EXPR:
 	  /* If this just a variant conversion or if the conversion doesn't
@@ -1487,7 +1487,7 @@  build_unary_op (enum tree_code op_code,
 	case CONST_DECL:
 	  operand = DECL_CONST_CORRESPONDING_VAR (operand);
 
-	  /* fall through */
+	  /* ... fall through ... */
 
 	default:
 	common:
@@ -1648,7 +1648,7 @@  build_unary_op (enum tree_code op_code,
 	  }
       }
 
-      /* fall through */
+      /* ... fall through ... */
 
     default:
       gcc_assert (operation_type == base_type);