From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 36241 invoked by alias); 14 Oct 2019 16:34:27 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 35515 invoked by uid 89); 14 Oct 2019 16:34:26 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.5 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,SPF_PASS autolearn=ham version=3.3.1 spammy=T, desktop, Client, underline X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 14 Oct 2019 16:34:16 +0000 Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 3355EAC38; Mon, 14 Oct 2019 16:34:14 +0000 (UTC) Date: Mon, 14 Oct 2019 16:35:00 -0000 From: Michael Matz To: David Malcolm cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH 2/2] Documentation hyperlinks for [-Wname-of-option] (PR 87488) In-Reply-To: <20191010170614.2061-2-dmalcolm@redhat.com> Message-ID: References: <20191010170614.2061-1-dmalcolm@redhat.com> <20191010170614.2061-2-dmalcolm@redhat.com> User-Agent: Alpine 2.21 (LSU 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-IsSubscribed: yes X-SW-Source: 2019-10/txt/msg00998.txt.bz2 Hi, On Thu, 10 Oct 2019, David Malcolm wrote: > This patch uses the support for pretty-printing escaped URLs added in > the previous patch (PR 87488) so that in a sufficiently capable terminal > the [-Wname-of-option] emitted at the end of each diagnostic becomes a > hyperlink to the documentation for that option on the GCC website. > > I've tested it with Fedora 30's GNOME Terminal (3.32.2 with VTE 0.56.3); > the option text is printed with a dotted underline, hovering shows the > URL, and on right-clicking it offers menu items to visit/copy the URL. I don't think you can enable this by default as long as even the gist page only claims that 8 of 22 terminal emulators support this (and among those not supporting it are xterm, and all default terminal emulators of desktop environment other than GNOME). screen and tmux don't support it either, and the konsole feature request for this has a discussion about problems with the design from a security p.o.v. so there will probably be emulators that never implement this. That there's no way of detecting the capability, and, worse, that the normal default behaviour of terminals is to ignore unknown OCS sequences (which means that you don't see the URL but only the replacement text) also means information loss. Tieing the hyperlinks default capability to the colorize capability is bad as well, because colorization is an old concept, and better yet, supported by terminfo and termcap capabilities (hence can be translated meaningfully by terminal multiplexers), unlike the hyperlinking. So, for the above reasons IMHO the default can only be false. Ciao, Michael. > > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu. > > Committed to trunk as r276843. > > gcc/ChangeLog: > PR 87488 > * Makefile.in (CFLAGS-opts.o): Pass in DOCUMENTATION_ROOT_URL via > -D. > * configure.ac (--with-documentation-root-url): New option. > * configure: Regenerate. > * diagnostic-format-json.cc (json_end_diagnostic): If there is an > option URL, add it as a new string field of the diagnostic option. > * diagnostic.c (diagnostic_initialize): Initialize get_option_url. > (print_option_information): If get_option_url is non-NULL, call > it, and if the result is non-NULL, potentially emit an escape > sequence to markup the option text with the resulting URL. > * diagnostic.h (diagnostic_context::get_option_url): New callback. > * doc/invoke.texi (-fdiagnostics-format=): Add "option_url" to > example of JSON output. > * opts-diagnostic.h (get_option_url): New decl. > * opts.c (get_option_url): New function. > * toplev.c (general_init): Initialize the get_option_url callback. > > gcc/testsuite/ChangeLog: > PR 87488 > * c-c++-common/diagnostic-format-json-2.c: Expect an "option_url" > field. > * c-c++-common/diagnostic-format-json-3.c: Likewise. > * gfortran.dg/diagnostic-format-json-2.F90: Likewise. > * gfortran.dg/diagnostic-format-json-3.F90: Likewise. > * jit.dg/test-error-array-bounds.c (create_code): Ensure that > error messages don't contain escaped URLs. > --- > gcc/Makefile.in | 2 ++ > gcc/configure.ac | 14 ++++++++++++++ > gcc/diagnostic-format-json.cc | 11 +++++++++++ > gcc/diagnostic.c | 12 ++++++++++++ > gcc/diagnostic.h | 6 ++++++ > gcc/doc/invoke.texi | 1 + > gcc/opts-diagnostic.h | 3 +++ > gcc/opts.c | 21 +++++++++++++++++++++ > .../c-c++-common/diagnostic-format-json-2.c | 1 + > .../c-c++-common/diagnostic-format-json-3.c | 1 + > .../c-c++-common/diagnostic-format-json-4.c | 10 ++++++++-- > .../gfortran.dg/diagnostic-format-json-2.F90 | 1 + > .../gfortran.dg/diagnostic-format-json-3.F90 | 1 + > gcc/testsuite/jit.dg/test-error-array-bounds.c | 5 +++-- > gcc/toplev.c | 1 + > 15 files changed, 86 insertions(+), 4 deletions(-) > > diff --git a/gcc/Makefile.in b/gcc/Makefile.in > index 59adfaa..c82858f 100644 > --- a/gcc/Makefile.in > +++ b/gcc/Makefile.in > @@ -2142,6 +2142,8 @@ lto-wrapper$(exeext): $(LTO_WRAPPER_OBJS) libcommon-target.a $(LIBDEPS) > $(LTO_WRAPPER_OBJS) libcommon-target.a $(LIBS) > mv -f T$@ $@ > > +CFLAGS-opts.o += -DDOCUMENTATION_ROOT_URL=\"@DOCUMENTATION_ROOT_URL@\" > + > # Files used by all variants of C or by the stand-alone pre-processor. > > CFLAGS-c-family/c-opts.o += @TARGET_SYSTEM_ROOT_DEFINE@ > diff --git a/gcc/configure.ac b/gcc/configure.ac > index 54a6715..62f4b26 100644 > --- a/gcc/configure.ac > +++ b/gcc/configure.ac > @@ -960,6 +960,20 @@ AC_SUBST(CONFIGURE_SPECS) > ACX_PKGVERSION([GCC]) > ACX_BUGURL([https://gcc.gnu.org/bugs/]) > > +# Allow overriding the default URL for documentation > +AC_ARG_WITH(documentation-root-url, > + AS_HELP_STRING([--with-documentation-root-url=URL], > + [Root for documentation URLs]), > + [case "$withval" in > + yes) AC_MSG_ERROR([documentation root URL not specified]) ;; > + no) AC_MSG_ERROR([documentation root URL not specified]) ;; > + *) DOCUMENTATION_ROOT_URL="$withval" > + ;; > + esac], > + DOCUMENTATION_ROOT_URL="https://gcc.gnu.org/onlinedocs/gcc/" > +) > +AC_SUBST(DOCUMENTATION_ROOT_URL) > + > # Sanity check enable_languages in case someone does not run the toplevel > # configure # script. > AC_ARG_ENABLE(languages, > diff --git a/gcc/diagnostic-format-json.cc b/gcc/diagnostic-format-json.cc > index 53c3b63..eb99952 100644 > --- a/gcc/diagnostic-format-json.cc > +++ b/gcc/diagnostic-format-json.cc > @@ -154,6 +154,17 @@ json_end_diagnostic (diagnostic_context *context, diagnostic_info *diagnostic, > free (option_text); > } > > + if (context->get_option_url) > + { > + char *option_url = context->get_option_url (context, > + diagnostic->option_index); > + if (option_url) > + { > + diag_obj->set ("option_url", new json::string (option_url)); > + free (option_url); > + } > + } > + > /* If we've already emitted a diagnostic within this auto_diagnostic_group, > then add diag_obj to its "children" array. */ > if (cur_group) > diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c > index 467cc39..a29bcf1 100644 > --- a/gcc/diagnostic.c > +++ b/gcc/diagnostic.c > @@ -200,6 +200,7 @@ diagnostic_initialize (diagnostic_context *context, int n_opts) > context->option_enabled = NULL; > context->option_state = NULL; > context->option_name = NULL; > + context->get_option_url = NULL; > context->last_location = UNKNOWN_LOCATION; > context->last_module = 0; > context->x_data = NULL; > @@ -912,11 +913,22 @@ print_option_information (diagnostic_context *context, > > if (option_text) > { > + char *option_url = NULL; > + if (context->get_option_url) > + option_url = context->get_option_url (context, > + diagnostic->option_index); > pretty_printer *pp = context->printer; > pp_string (pp, " ["); > pp_string (pp, colorize_start (pp_show_color (pp), > diagnostic_kind_color[diagnostic->kind])); > + if (option_url) > + pp_begin_url (pp, option_url); > pp_string (pp, option_text); > + if (option_url) > + { > + pp_end_url (pp); > + free (option_url); > + } > pp_string (pp, colorize_stop (pp_show_color (pp))); > pp_character (pp, ']'); > free (option_text); > diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h > index f0ea8e8..91e4c50 100644 > --- a/gcc/diagnostic.h > +++ b/gcc/diagnostic.h > @@ -194,6 +194,12 @@ struct diagnostic_context > May be passed 0 as well as the index of a particular option. */ > char *(*option_name) (diagnostic_context *, int, diagnostic_t, diagnostic_t); > > + /* Client hook to return a URL describing the option that controls > + a diagnostic. Returns malloced memory. May return NULL if no URL > + is available. May be passed 0 as well as the index of a > + particular option. */ > + char *(*get_option_url) (diagnostic_context *, int); > + > /* Auxiliary data for client. */ > void *x_data; > > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index bdbcd95..085e8b0 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -4116,6 +4116,7 @@ might be printed in JSON form (after formatting) like this: > ], > "message": "this \u2018if\u2019 clause does not guard...", > "option": "-Wmisleading-indentation", > + "option_url": "https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wmisleading-indentation", > "children": [ > @{ > "kind": "note", > diff --git a/gcc/opts-diagnostic.h b/gcc/opts-diagnostic.h > index 08110af..3f58ae9 100644 > --- a/gcc/opts-diagnostic.h > +++ b/gcc/opts-diagnostic.h > @@ -22,4 +22,7 @@ along with GCC; see the file COPYING3. If not see > > extern char *option_name (diagnostic_context *context, int option_index, > diagnostic_t orig_diag_kind, diagnostic_t diag_kind); > + > +extern char *get_option_url (diagnostic_context *context, int option_index); > + > #endif > diff --git a/gcc/opts.c b/gcc/opts.c > index f98f3f4..5ef6bf7 100644 > --- a/gcc/opts.c > +++ b/gcc/opts.c > @@ -3236,3 +3236,24 @@ option_name (diagnostic_context *context, int option_index, > else > return NULL; > } > + > +/* Return malloced memory for a URL describing the option OPTION_INDEX > + which enabled a diagnostic (context CONTEXT). */ > + > +char * > +get_option_url (diagnostic_context *, int option_index) > +{ > + if (option_index) > + /* DOCUMENTATION_ROOT_URL should be supplied via -D by the Makefile > + (see --with-documentation-root-url). > + > + Expect an anchor of the form "index-Wfoo" e.g. > + , and thus an id within > + the URL of "#index-Wformat". */ > + return concat (DOCUMENTATION_ROOT_URL, > + "Warning-Options.html", > + "#index", cl_options[option_index].opt_text, > + NULL); > + else > + return NULL; > +} > diff --git a/gcc/testsuite/c-c++-common/diagnostic-format-json-2.c b/gcc/testsuite/c-c++-common/diagnostic-format-json-2.c > index 239c75e..557ccf8 100644 > --- a/gcc/testsuite/c-c++-common/diagnostic-format-json-2.c > +++ b/gcc/testsuite/c-c++-common/diagnostic-format-json-2.c > @@ -10,6 +10,7 @@ > /* { dg-regexp "\"kind\": \"warning\"" } */ > /* { dg-regexp "\"message\": \"#warning message\"" } */ > /* { dg-regexp "\"option\": \"-Wcpp\"" } */ > +/* { dg-regexp "\"option_url\": \"https:\[^\n\r\"\]*#index-Wcpp\"" } */ > > /* { dg-regexp "\"caret\": \{" } */ > /* { dg-regexp "\"file\": \"\[^\n\r\"\]*diagnostic-format-json-2.c\"" } */ > diff --git a/gcc/testsuite/c-c++-common/diagnostic-format-json-3.c b/gcc/testsuite/c-c++-common/diagnostic-format-json-3.c > index 1c54eca..378205c 100644 > --- a/gcc/testsuite/c-c++-common/diagnostic-format-json-3.c > +++ b/gcc/testsuite/c-c++-common/diagnostic-format-json-3.c > @@ -10,6 +10,7 @@ > /* { dg-regexp "\"kind\": \"error\"" } */ > /* { dg-regexp "\"message\": \"#warning message\"" } */ > /* { dg-regexp "\"option\": \"-Werror=cpp\"" } */ > +/* { dg-regexp "\"option_url\": \"https:\[^\n\r\"\]*#index-Wcpp\"" } */ > > /* { dg-regexp "\"caret\": \{" } */ > /* { dg-regexp "\"file\": \"\[^\n\r\"\]*diagnostic-format-json-3.c\"" } */ > diff --git a/gcc/testsuite/c-c++-common/diagnostic-format-json-4.c b/gcc/testsuite/c-c++-common/diagnostic-format-json-4.c > index 1c3b034..2738be6 100644 > --- a/gcc/testsuite/c-c++-common/diagnostic-format-json-4.c > +++ b/gcc/testsuite/c-c++-common/diagnostic-format-json-4.c > @@ -30,13 +30,12 @@ int test (void) > /* { dg-regexp "\"line\": 8" } */ > /* { dg-regexp "\"column\": 10" } */ > > -/* { dg-regexp "\"locations\": \[\[\{\}, \]*\]" } */ > - > /* The outer diagnostic. */ > > /* { dg-regexp "\"kind\": \"warning\"" } */ > /* { dg-regexp "\"message\": \"this 'if' clause does not guard...\"" } */ > /* { dg-regexp "\"option\": \"-Wmisleading-indentation\"" } */ > +/* { dg-regexp "\"option_url\": \"https:\[^\n\r\"\]*#index-Wmisleading-indentation\"" } */ > > /* { dg-regexp "\"caret\": \{" } */ > /* { dg-regexp "\"file\": \"\[^\n\r\"\]*diagnostic-format-json-4.c\"" } */ > @@ -48,6 +47,13 @@ int test (void) > /* { dg-regexp "\"line\": 6" } */ > /* { dg-regexp "\"column\": 4" } */ > > +/* More from the nested diagnostic (we can't guarantee what order the > + "file" keys are consumed). */ > + > +/* { dg-regexp "\"locations\": \[\[\{\}, \]*\]" } */ > + > +/* More from the outer diagnostic. */ > + > /* { dg-regexp "\"locations\": \[\[\{\}, \]*\]" } */ > > /* { dg-regexp "\"children\": \[\[\{\}, \]*\]" } */ > diff --git a/gcc/testsuite/gfortran.dg/diagnostic-format-json-2.F90 b/gcc/testsuite/gfortran.dg/diagnostic-format-json-2.F90 > index a6d27d9..bebcf68 100644 > --- a/gcc/testsuite/gfortran.dg/diagnostic-format-json-2.F90 > +++ b/gcc/testsuite/gfortran.dg/diagnostic-format-json-2.F90 > @@ -10,6 +10,7 @@ > ! { dg-regexp "\"kind\": \"warning\"" } > ! { dg-regexp "\"message\": \"#warning message\"" } > ! { dg-regexp "\"option\": \"-Wcpp\"" } > +! { dg-regexp "\"option_url\": \"\[^\n\r\"\]*#index-Wcpp\"" } > > ! { dg-regexp "\"caret\": \{" } > ! { dg-regexp "\"file\": \"\[^\n\r\"\]*diagnostic-format-json-2.F90\"" } > diff --git a/gcc/testsuite/gfortran.dg/diagnostic-format-json-3.F90 b/gcc/testsuite/gfortran.dg/diagnostic-format-json-3.F90 > index 702a937..7ab78eb 100644 > --- a/gcc/testsuite/gfortran.dg/diagnostic-format-json-3.F90 > +++ b/gcc/testsuite/gfortran.dg/diagnostic-format-json-3.F90 > @@ -10,6 +10,7 @@ > ! { dg-regexp "\"kind\": \"error\"" } > ! { dg-regexp "\"message\": \"#warning message\"" } > ! { dg-regexp "\"option\": \"-Werror=cpp\"" } > +! { dg-regexp "\"option_url\": \"\[^\n\r\"\]*#index-Wcpp\"" } > > ! { dg-regexp "\"caret\": \{" } > ! { dg-regexp "\"file\": \"\[^\n\r\"\]*diagnostic-format-json-3.F90\"" } > diff --git a/gcc/testsuite/jit.dg/test-error-array-bounds.c b/gcc/testsuite/jit.dg/test-error-array-bounds.c > index 889c517..cd9361f 100644 > --- a/gcc/testsuite/jit.dg/test-error-array-bounds.c > +++ b/gcc/testsuite/jit.dg/test-error-array-bounds.c > @@ -20,9 +20,10 @@ create_code (gcc_jit_context *ctxt, void *user_data) > gcc_jit_context_add_command_line_option (ctxt, "-Warray-bounds"); > gcc_jit_context_add_command_line_option (ctxt, "-ftree-vrp"); > > - /* Ensure that the error message doesn't contain colorization codes, > - even if run at a TTY. */ > + /* Ensure that the error message doesn't contain colorization codes > + or escaped URLs, even if run at a TTY. */ > gcc_jit_context_add_command_line_option (ctxt, "-fdiagnostics-color=never"); > + gcc_jit_context_add_command_line_option (ctxt, "-fdiagnostics-urls=never"); > > gcc_jit_type *char_type = > gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_CHAR); > diff --git a/gcc/toplev.c b/gcc/toplev.c > index d741a66..df4ca01 100644 > --- a/gcc/toplev.c > +++ b/gcc/toplev.c > @@ -1110,6 +1110,7 @@ general_init (const char *argv0, bool init_signals) > global_dc->option_enabled = option_enabled; > global_dc->option_state = &global_options; > global_dc->option_name = option_name; > + global_dc->get_option_url = get_option_url; > > if (init_signals) > { >