Fix memory leaks in tree-ssa-uninit.c

On 11/19/2015 01:50 AM, Joseph Myers wrote:
> 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)
>> -  {}
>> +  {
>> +  }

Hi Joseph.

Agree with you that logical expressions are not formatted by the tool ideally,
human interaction is highly needed. The same rule can be applied to function arguments,
where we have mixture of approaches:

a) maximally fill-up a line
b) logically separate group of arguments
c) put each argument on a separate line if it does not fit to the single line

As the patch hasn't been installed, I'm attaching v2, where I preserve original grouping of a function
arguments and logic expressions that are made of multiple operands should be fine.

>
> Is that really what we want?

The hunk is reverted.

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

You are right, however as the original coding style was really broken, it was much easier
to use the tool and clean-up fall-out.

Waiting for thoughts related to v2.

Thanks,
Martin

Fix memory leaks in tree-ssa-uninit.c

Commit Message

Comments

Patch