On Wed, Aug 25, 2021 at 10:03 PM Martin Sebor <mse...@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,
Richard.
> Tested on x86_64-linux.
>
> Martin
