On Wed, 2020-04-29 at 15:52 +0200, Jakub Jelinek wrote:
> On Wed, Apr 29, 2020 at 09:24:09AM -0400, David Malcolm wrote:
[...]
> > There's some pre-existing repetition between arm, aarch64, and
> > rs6000,
> > in which this patch has to make the same changes in multiple
> > places.
> > Could these be consolidated e.g.
> >
> > void maybe_emit_gcc10_param_passing_abi_warning (tree type)
> > {
> > /* something here; not sure what */
> > }
> >
> > That said it's a pre-existing thing.
>
> I'd prefer not to at this point, all that could be commonized are
> the two inform calls perhaps with a bool or enum arg which one it is,
> but usually the backends prefer to keep their -Wpsabi stuff visible
> there.
> Yes, it affects 4 targets (s390 too; haven't touched that one,
> because
> there is a pending patch with two alternatives).
One other benefit is to move the allocation/free of the URL string so
that it's guarded by the same condition as the "inform" call.
Is there a chance this patch could be doing a bunch of extra allocation
and freeing of URL strings that never get used? How often does this
code get called?
Idea: introduce a static allocation for this
const char *
get_url_for_gcc_10_empty_base ()
{
static const char *url;
if (!url)
url = get_changes_url ("gcc-10/changes.html#empty_base");
return url;
}
or somesuch?
> > > + const char *url
> > > + = get_changes_url ("gcc-10/changes.html#empty_base");
> >
> > "url" is declared here (and elsewhere) as a "const char *", but
> > later
> > freed; that seems like a violation of const-
> > correctness. Hopefully
> > we
> > emit a warning about that.
>
> Changed to char *, one could use free (CONST_CAST (char *, url)) too
> though.
>
> > > @@ -6423,11 +6425,14 @@ aapcs_vfp_is_call_or_return_candidate (e
> > > if (warn_psabi_flags & WARN_PSABI_NO_UNIQUE_ADDRESS)
> > > inform (input_location, "parameter passing for argument
> > > of "
> > > "type %qT with %<[[no_unique_address]]%>
> > > members "
> > > - "changed in GCC 10.1", TYPE_MAIN_VARIANT
> > > (type));
> > > + "changed in %{GCC 10.1%}",
> > > + TYPE_MAIN_VARIANT (type), url);
> >
> > A different bikeshed: it might be better style for the hyperlink to
> > cover the text "%{changed in GCC 10.1%}" given that the link is
> > describing a specific change in GCC 10.1 rather than GCC 10.1
> > itself.
>
> I think I can't do that, because there are two inform calls and while
> one
> has the changed in GCC 10.1 in that order, the other doesn't, as it
> has
> changed to match C++14 in GCC 10.1. Additionally, for translations,
> I think
> some translators will need to reorder the words in various ways, and
> am not
> sure what they would do if e.g. the translated changed word is way
> appart
> from in GCC 10.1. One could use
> ... type %1$qT ... %2${changed%} ... %2${in GCC 10.1%} perhaps, which
> would mean two separate underlined words, but not sure if they'd
> figure it
> out.
> Is just %{in GCC 10.1%} good enough (in the patch below)?
Yes.
> I've also included the missing free (url); in rs6000-call.c Richard
> Sandiford reported.
Thanks.
> 2020-04-29 Jakub Jelinek <ja...@redhat.com>
>
> * configure.ac (-with-changes-root-url): New configure option,
> defaulting to https://gcc.gnu.org/.
> * Makefile.in (CFLAGS-opts.o): Define CHANGES_ROOT_URL for
> opts.c.
> * pretty-print.c (get_end_url_string): New function.
> (pp_format): Handle %{ and %} for URLs.
> (pp_begin_url): Use pp_string instead of pp_printf.
> (pp_end_url): Use get_end_url_string.
> * opts.h (get_changes_url): Declare.
> * opts.c (get_changes_url): New function.
> * config/rs6000/rs6000-call.c: Include opts.h.
> (rs6000_discover_homogeneous_aggregate): Use %{in GCC 10.1%}
> instead
> of just in GCC 10.1 in diagnostics and add URL.
> * config/arm/arm.c (aapcs_vfp_is_call_or_return_candidate):
> Likewise.
> * config/aarch64/aarch64.c
> (aarch64_vfp_is_call_or_return_candidate):
> Likewise.
> * configure: Regenerated.
>
> * c-format.c (PP_FORMAT_CHAR_TABLE): Add %{ and %}.
>
[...]
LGTM, with the caveat about allocation noted above.
Thanks
Dave
