On Sat, 2020-02-01 at 07:30 +0000, Bernd Edlinger wrote:
>
> On 1/31/20 11:06 PM, David Malcolm wrote:
> > 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.
[..]
Thanks for the updated patch; here are some notes:
> diff --git a/gcc/diagnostic-color.c b/gcc/diagnostic-color.c
> index d554795..f406139 100644
> --- a/gcc/diagnostic-color.c
> +++ b/gcc/diagnostic-color.c
> @@ -216,6 +216,10 @@ should_colorize (void)
> && GetConsoleMode (h, &m);
> #else
> char const *t = getenv ("TERM");
> + /* Do not enable colors when TMUX is detected, since that is
> + known to not work reliably. */
> + if (t && strcmp (t, "screen") && getenv ("TMUX"))
> + return false;
> return t && strcmp (t, "dumb") != 0 && isatty (STDERR_FILENO);
> #endif
> }
Does our existing colorization code not work with TMUX, or is it just the
new URLs that are broken? Segher?
> @@ -239,20 +243,86 @@ colorize_init (diagnostic_color_rule_t rule)
> }
> }
>
> +/* Return URL_FORMAT_XXX which tells how we should emit urls
> + when in always mode.
> + We use GCC_URLS and if that is not defined TERM_URLS.
> + If neither is defined the feature is enabled by default. */
> +
> +static diagnostic_url_format
> +parse_gcc_urls()
Perhaps rename this to parse_env_vars_for_urls or somesuch, given that
it's not just GCC_URLS being parsed?
> +/* 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;
> +
> + /* First check the terminal is capable to print color escapes,
^^^^^^^^^^^^^^^^^^^
grammar nit: "is capable of printing"
> + if not URLs won't work either. */
> + if (!should_colorize ())
> + return false;
> +
> + p = getenv ("COLORTERM");
> + if (p == NULL)
> + return true;
Is this part of the rejection of terminals that can't print color
escapes, or is this some other heuristic based on the discussion?
> + /* xfce4-terminal is known to not implement URLs at this time.
> + Recently new installations will safely ignore the URL escape
> + sequences, but a large number of legacy installations print
> + garbage when URLs are printed. Therefore we loose nothing by
^^^^^
"loose" -> "lose"
If you have version numbers handy, it might be good to mention those
in this comment, since references to "at this time" and "new" can
get dated.
> + disabling this feature for that specific terminal type. */
> + if (!strcmp (p, "xfce4-terminal"))
> + return false;
> +
> + return true;
> +#endif
> +}
> +
> /* Determine if URLs should be enabled, based on RULE.
> This reuses the logic for colorization. */
Maybe this:
/* Determine if URLs should be enabled, based on RULE,
and, if so, which format to use.
This reuses the logic for colorization. */
> -bool
> -diagnostic_urls_enabled_p (diagnostic_url_rule_t rule)
> +diagnostic_url_format
> +diagnostic_urls_enabled (diagnostic_url_rule_t rule)
[...]
> diff --git a/gcc/diagnostic-url.h b/gcc/diagnostic-url.h
> index 6be0569..22253fd 100644
> --- a/gcc/diagnostic-url.h
> +++ b/gcc/diagnostic-url.h
> @@ -31,6 +31,20 @@ typedef enum
> DIAGNOSTICS_URL_AUTO = 2
> } diagnostic_url_rule_t;
>
> -extern bool diagnostic_urls_enabled_p (diagnostic_url_rule_t);
> +/* Tells how URLs are to be emitted:
> + - URL_FORMAT_NONE means no URLs shall be emitted.
> + - URL_FORMAT_ST means use ST string termination.
> + - URL_FROMAT_BEL means use BEL string termination,
> + which is the default. */
> +enum diagnostic_url_format
> +{
> + URL_FORMAT_NONE,
> + URL_FORMAT_ST,
> + URL_FORMAT_BEL
> +};
There's a typo in the above, but I think it's better to document
the values inline, so how about:
/* Tells whether URLs should be emitted, and, if so, how to
terminate strings within the escape sequence. */
enum diagnostic_url_format
{
/* No URLs shall be emitted. */
URL_FORMAT_NONE,
/* Use ST string termination. */
URL_FORMAT_ST,
/* Use BEL string termination. */
URL_FORMAT_BEL
};
> +#define URL_FORMAT_DEFAULT URL_FORMAT_BEL
Can this be a const rather than a #define?
[...]
> diff --git a/gcc/pretty-print.h b/gcc/pretty-print.h
> index 001468c9..a0d273b 100644
> --- a/gcc/pretty-print.h
> +++ b/gcc/pretty-print.h
[...]
> @@ -279,7 +280,7 @@ public:
> bool show_color;
>
> /* Nonzero means that URLs should be emitted. */
> - bool show_urls;
> + diagnostic_url_format show_urls;
> };
How about renaming this field to "url_format"? Maybe:
/* Whether URLs should be emitted, and which terminator to use. */
diagnostic_url_format url_format;
Hope this is constructive
Dave
