[PATCH] improve note location and refactor warn_uninit

Jeff Law jeffreyalaw@gmail.com
Fri Aug 27 21:41:28 GMT 2021



On 8/26/2021 1:00 AM, Richard Biener via Gcc-patches wrote:
> On Wed, Aug 25, 2021 at 10:03 PM Martin Sebor <msebor@gmail.com> wrote:
>> Richard, some time ago you mentioned you'd had trouble getting
>> -Wuninitialized to print the note pointing to the uninitialized
>> variable.  I said I'd look into it, and so I did.  The attached
>> patch simplifies the warn_uninit() function to get rid of some
>> redundant cruft: besides a few pointless null tests and
>> a duplicate argument it also does away with ancient code that's
>> responsible for the problem you saw.  It turns out that tests
>> for the problem are already in the test suite but have been
>> xfailed since the day they were added, so the patch simply
>> removes the xfails.  I'll go ahead and commit this cleanup
>> as obvious later this week unless you have suggestions for
>> further changes.
> I do see lots of useful refactoring.
>
> -  if (context != NULL && gimple_has_location (context))
> -    location = gimple_location (context);
> -  else if (phiarg_loc != UNKNOWN_LOCATION)
> -    location = phiarg_loc;
> -  else
> -    location = DECL_SOURCE_LOCATION (var);
> +  /* Use either the location of the read statement or that of the PHI
> +     argument, or that of the uninitialized variable, in that order,
> +     whichever is valid.  */
> +  location_t location = gimple_location (context);
> +  if (location == UNKNOWN_LOCATION)
> +    {
> +      if (phi_arg_loc == UNKNOWN_LOCATION)
> +       location = DECL_SOURCE_LOCATION (var);
> +      else
> +       location = phi_arg_loc;
> +    }
>
> the comment is an improvement but I think the original flow is
> easier to follow ;)
>
> -      if (xloc.file != floc.file
> -         || linemap_location_before_p (line_table, location, cfun_loc)
> -         || linemap_location_before_p (line_table, cfun->function_end_locus,
> -                                       location))
> -       inform (DECL_SOURCE_LOCATION (var), "%qD was declared here", var);
> ...
> +  inform (var_loc, "%qD was declared here", var);
>
> and this is the actual change that "fixes" the missing diagnostics?  Can
> you explain what the reason for the original sing-and-dance was?  It
> looks like it tries to print the 'was declared here' only for locations
> within the current function (but then it probably intended to do that
> on the DECL_SOURCE_LOCATION (var), not on whatever we are
> choosing now)?
>
> That said, I'd prefer if you can commit the refactoring independently
> of this core change and can try to dig why this was added and what
> it was supposed to do.
Thanks.  And just to be 100% clear, all I was objecting to was the 
assertion that the patch was obvious and that it could be committed 
under the obvious rule.
jeff


More information about the Gcc-patches mailing list