Re: [PATCH] Fix memory leaks in tree-ssa-uninit.c

[Joseph Myers <joseph at codesourcery dot com>]

I don't think all the reformattings here are things we want to do globally 
for most source files.  E.g.

> @@ -75,18 +74,17 @@ get_mask_first_set_bit (unsigned mask)
>  static bool
>  has_undefined_value_p (tree t)
>  {
> -  return (ssa_undefined_value_p (t)
> -          || (possibly_undefined_names
> -              && possibly_undefined_names->contains (t)));
> +  return ssa_undefined_value_p (t)
> +	 || (possibly_undefined_names
> +	     && possibly_undefined_names->contains (t));

This seems plain wrong.  The GNU Coding Standards explicitly say that in 
such cases where a binary operator goes on the start of the next line, you 
should insert extra parentheses so that Emacs will get the indentation 
right (even though otherwise parentheses shouldn't be used with "return").  
So the old code was better.

>  static void
> -warn_uninit (enum opt_code wc, tree t, tree expr, tree var,
> -	     const char *gmsgid, void *data, location_t phiarg_loc)
> +warn_uninit (enum opt_code wc, tree t, tree expr, tree var, const char *gmsgid,
> +	     void *data, location_t phiarg_loc)

Just because it's OK to go up to an 80- or 79-column limit if necessary 
doesn't mean code should be reformatted to do so.  Breaking a bit before 
that is perfectly reasonable in general (and in some cases, the choice of 
where to break may be based on logical grouping of arguments).

> @@ -135,10 +133,9 @@ warn_uninit (enum opt_code wc, tree t, tree expr, tree var,
>  
>    /* TREE_NO_WARNING either means we already warned, or the front end
>       wishes to suppress the warning.  */
> -  if ((context
> -       && (gimple_no_warning_p (context)
> -	   || (gimple_assign_single_p (context)
> -	       && TREE_NO_WARNING (gimple_assign_rhs1 (context)))))
> +  if ((context && (gimple_no_warning_p (context)
> +		   || (gimple_assign_single_p (context)
> +		       && TREE_NO_WARNING (gimple_assign_rhs1 (context)))))

I think in cases such as this, putting the operator && on the start of the 
next line to make subsequent lines less-indented makes sense.  Again, the 
new formatting isn't incorrect, but that doesn't make it a desirable 
change.

> @@ -217,24 +212,20 @@ warn_uninitialized_vars (bool warn_possibly_uninitialized)
>  	     so must be limited which means we would miss warning
>  	     opportunities.  */
>  	  use = gimple_vuse (stmt);
> -	  if (use
> -	      && gimple_assign_single_p (stmt)
> -	      && !gimple_vdef (stmt)
> +	  if (use && gimple_assign_single_p (stmt) && !gimple_vdef (stmt)
>  	      && SSA_NAME_IS_DEFAULT_DEF (use))

I think there may be a preference, where it's necessary to use multiple 
lines (if the whole condition doesn't fit on one line) for a condition 
like this, for each "&& condition" to go on its own line, rather than some 
lines having multiple conditions.

>  	      /* Do not warn if it can be initialized outside this function.  */
> -	      if (TREE_CODE (base) != VAR_DECL
> -		  || DECL_HARD_REGISTER (base)
> +	      if (TREE_CODE (base) != VAR_DECL || DECL_HARD_REGISTER (base)
>  		  || is_global_var (base))

Likewise.

> @@ -2393,15 +2338,28 @@ class pass_late_warn_uninitialized : public gimple_opt_pass
>  public:
>    pass_late_warn_uninitialized (gcc::context *ctxt)
>      : gimple_opt_pass (pass_data_late_warn_uninitialized, ctxt)
> -  {}
> +  {
> +  }

Is that really what we want?

I think those examples are sufficient to illustrate the problems with 
automatic conversion from one correct format to another correct format (as 
opposed to fixing cases where the formatting is unambiguously incorrect, 
which covers plenty of the changes in this patch).

-- 
Joseph S. Myers
joseph@codesourcery.com