On Fri, 2020-01-31 at 16:59 +0000, Bernd Edlinger wrote: > Hi, > > this is patch is heavily based on David's original patch here: > https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01409.html > > and addresses Jakub's review comments here: > https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01412.html > > So I hope you don't mind, when I pick up this patch since there > was not much activity recently on this issue, so I assumed you > would appreciate some help.
Thanks Bernd; sorry, I got distracted by analyzer bug-fixing. It's appreciated. > I will try to improve the patch a bit, and hope you are gonna like > it. I agree that this feature is fine, and should be enabled by > default, and just if it is positively clear that it won't work, > disabled in the auto mode. > > Also as requested by Jakub this tries to be more compatible to > GCC_COLORS define, and adds the capability to switch between ST > and BEL string termination and also have a way to disable urls > even with -fdiagnostics-urls=always (like GCC_COLORS= disables > colors). > > In addition to that I propose to use GCC_URLS and if that > is not defined use TERM_URLS to control that feature for > all applications willing to support that. I think I like the overall idea. > Since I have seen much garbage from the URLs in the xfce4-terminal > 0.6.3 > admittedly an old Ubuntu installation, but still in LTS status, > and no attempt from the xfc4-terminal to _ever_ implement URLs, I > would > like to detect the xfce4-terminal, and use that in auto mode > to switch off the URLs feature, since in best case the URLs will > just be ignored, but can and will often create garbage on the screen. > > Likewise on MinGW, since the windows 10 cmd prompt does at best > ignore the URLs, but windows terminal and windows 7 cmd prompt > print garbage when URLs are output. That sounds reasonable, but we should document those exclusions, I think. I think the ChangeLog should also refer to "PR other/93168": https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93168 > diff --git a/gcc/diagnostic-color.c b/gcc/diagnostic-color.c > index d554795..ad84d1f 100644 > --- a/gcc/diagnostic-color.c > +++ b/gcc/diagnostic-color.c > @@ -239,6 +239,56 @@ colorize_init (diagnostic_color_rule_t rule) > } > } > > +bool diagnostic_urls_use_st = false; I don't like global variables (a pet peeve of mine); can you make this be a field of the pretty_printer instead? Even better: convert pretty_printer::show_urls from a bool to a new enum; something like: enum diagnostic_url_format { URL_FORMAT_NONE, URL_FORMAT_ST, URL_FORMAT_BEL }; with suitable leading comments (probably a good place to summarize some of the compatibility info within the source). Then we could verify the differences between ST and BEL in the selftests, and also not have the selftests be affected by env vars. > +/* Return true if we should use urls when in always mode, false otherwise. > + Additionally initialize diagnostic_urls_use_st to true if ST escapes > + should be used and false for BEL escapes. */ Maybe have this return that 3-way enum instead? > + > +static bool > +parse_gcc_urls() > +{ > + const char *p; > + > + p = getenv ("GCC_URLS"); /* Plural! */ > + if (p == NULL) > + p = getenv ("TERM_URLS"); > + > + if (p == NULL) > + return true; > + if (*p == '\0') > + return false; > + > + if (!strcmp (p, "no")) > + return false; > + if (!strcmp (p, "st")) > + diagnostic_urls_use_st = true; > + else if (!strcmp (p, "bel")) > + diagnostic_urls_use_st = false; > + return true; > +} > + > +/* Return true if we should use urls when in auto mode, false otherwise. */ > + > +static bool > +auto_enable_urls () > +{ > +#ifdef __MINGW32__ > + return false; > +#else > + const char *p; > + > + if (!should_colorize ()) > + return false; > + p = getenv ("COLORTERM"); > + if (p == NULL) > + return true; > + if (!strcmp (p, "xfce4-terminal")) > + return false; > + return true; > +#endif > +} I think this logic warrants a comment for each of the exclusions: why are we excluding them? I'd prefer to capture that in the source, rather than just in the mailing list. > /* Determine if URLs should be enabled, based on RULE. > This reuses the logic for colorization. */ > > @@ -250,9 +300,12 @@ diagnostic_urls_enabled_p (diagnostic_url_rule_t rule) > case DIAGNOSTICS_URL_NO: > return false; > case DIAGNOSTICS_URL_YES: > - return true; > + return parse_gcc_urls (); > case DIAGNOSTICS_URL_AUTO: > - return should_colorize (); > + if (auto_enable_urls ()) > + return parse_gcc_urls (); > + else > + return false; > default: > gcc_unreachable (); > } > diff --git a/gcc/diagnostic-url.h b/gcc/diagnostic-url.h > index 6be0569..06b7784 100644 > --- a/gcc/diagnostic-url.h > +++ b/gcc/diagnostic-url.h > @@ -32,5 +32,6 @@ typedef enum > } diagnostic_url_rule_t; > > extern bool diagnostic_urls_enabled_p (diagnostic_url_rule_t); > +extern bool diagnostic_urls_use_st; > > #endif /* ! GCC_DIAGNOSTIC_URL_H */ > diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c > index 3386f07..55e323f 100644 > --- a/gcc/diagnostic.c > +++ b/gcc/diagnostic.c > @@ -260,8 +260,23 @@ diagnostic_color_init (diagnostic_context *context, int > value /*= -1 */) > void > diagnostic_urls_init (diagnostic_context *context, int value /*= -1 */) > { > + /* value == -1 is the default value. */ > if (value < 0) > - value = DIAGNOSTICS_COLOR_DEFAULT; > + { > + /* If DIAGNOSTICS_URLS_DEFAULT is -1, default to > + -fdiagnostics-urls=auto if GCC_URLS or TERM_URLS is in the > + environment, otherwise default to -fdiagnostics-urls=never, > + for other values default to that > + -fdiagnostics-urls={never,auto,always}. */ > + if (DIAGNOSTICS_URLS_DEFAULT == -1) > + { > + if (!getenv ("GCC_URLS") && !getenv ("TERM_URLS")) > + return; > + value = DIAGNOSTICS_URL_AUTO; > + } > + else > + value = DIAGNOSTICS_URLS_DEFAULT; > + } > > context->printer->show_urls > = diagnostic_urls_enabled_p ((diagnostic_url_rule_t) value); > diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi > index 6ffafac..f3f63be 100644 > --- a/gcc/doc/install.texi > +++ b/gcc/doc/install.texi > @@ -2100,6 +2100,15 @@ where @samp{auto} is the default. @samp{auto-if-env} > means that > is present and non-empty in the environment, and > @option{-fdiagnostics-color=never} otherwise. > > +@item --with-diagnostics-urls=@var{choice} > +Tells GCC to use @var{choice} as the default for @option{-fdiagnostics-urls=} > +option (if not used explicitly on the command line). @var{choice} > +can be one of @samp{never}, @samp{auto}, @samp{always}, and > @samp{auto-if-env} > +where @samp{auto} is the default. @samp{auto-if-env} means that > +@option{-fdiagnostics-urls=auto} will be the default if @env{GCC_URLS} > +or @env{TERM_URLS} is present and non-empty in the environment, and > +@option{-fdiagnostics-urls=never} otherwise. I realize that you're adapting what I wrote, but I realize now it's a little unclear: are those environment vars checked at configure time, or when GCC is run? I believe that it's the latter, but this doc could be misread as if it's the former. [...] > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index 2cd8d7e..4ebb411 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -4032,14 +4032,25 @@ arguments in the C++ frontend. > @item -fdiagnostics-urls[=@var{WHEN}] > @opindex fdiagnostics-urls > @cindex urls > +@vindex GCC_URLS @r{environment variable} Should also have a vindex for TERM_URLS here. > Use escape sequences to embed URLs in diagnostics. For example, when > @option{-fdiagnostics-show-option} emits text showing the command-line > option controlling a diagnostic, embed a URL for documentation of that > option. > > @var{WHEN} is @samp{never}, @samp{always}, or @samp{auto}. > -The default is @samp{auto}, which means to use URL escape sequences only > -when the standard error is a terminal. > +@samp{auto} means to use URL escape sequences only when the standard error > +is a terminal, and @env{TERM} is not set to "dumb". > + > +The default depends on how the compiler has been configured, > +it can be any of the above @var{WHEN} options or also @samp{never} > +if neither @env{GCC_URLS} nor @env{TERM_URLS} environment variable is > +present in the environment, and @samp{auto} otherwise. I found myself getting confused reading the above. I this would be clearer if we start a new sentence or para at the "or also" above. How about something like: The default depends on how the compiler has been configured. It can be any of the above @var{WHEN} options. GCC can also be configured (via the @option{--with-diagnostics-urls=auto-if-env} configure-time option) so that the default is affected by environment variables. Under such a configuration, GCC defaults to using @samp{auto} if either @env{GCC_URLS} or @env{TERM_URLS} environment variables are present in the environment, or @samp{never} if neither are. Did I correctly characterize the behavior? Is that clearer? > +If @env{GCC_URLS} set to empty or "no" do not embed URLs in diagnostics. > +If set to "st", URLs use ST escape sequences. > +If set to "bel", the default, URLs use BEL escape sequences. > +Any other non-empty value enables the feature. > +If @env{GCC_URLS} is not set, use @env{TERM_URLS} as a fallback. This also could use a little rewording. The situation is quite complex in that we have configure-time defaults, command-line defaults, and environment variables interacting with each other. Am I right in thinking that: (a) with the configure-time default of --with-diagnostics-urls=auto that you get URLs if at a tty, but you can set GCC_URLS=no to turn them off, or can override that with the -fdiagnostics-urls=WHEN (b) with the configure-time default of --with-diagnostics-urls=auto-if-env that you don't get URLs if at tty unless you set GCC_URLS in your env. and that some known not-working are always excluded? Hence (a) presumably makes a good default for a bleeding-edge distribution, and (b) may makes a good default for a conservative distribution which may generally have older terminals. We should also document the exclusions here (e.g. xfce-terminal). [...] Thanks again for working on this Dave
