[PATCH] Add --with-diagnostics-urls configuration option and GCC_URLS/TERM_URLS env var

Bernd Edlinger bernd.edlinger@hotmail.de
Sun Feb 2 14:07:00 GMT 2020


On 2/2/20 1:28 AM, Sandra Loosemore wrote:
> On 2/1/20 8:43 AM, Bernd Edlinger wrote:
> 
>> diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi
>> index 6ffafac..1d3fec5 100644
>> --- a/gcc/doc/install.texi
>> +++ b/gcc/doc/install.texi
>> @@ -2097,9 +2097,18 @@ 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-color=auto} will be the default if @code{GCC_COLORS}
>> -is present and non-empty in the environment, and
>> +is present and non-empty in the environment of the compiler, 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 of the
>> +compiler, and @option{-fdiagnostics-urls=never} otherwise.
>> +
> 
> It would be good to rewrite both of the above paragraphs to get rid of the future tense and put them in the active voice, like
> 
> @samp{auto-if-env} makes ... the default if ...
> 

done.

> Also @code{GCC_COLORS} ought to be @env{GCC_COLORS}, no?
> 

right.

>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
>> index b8ba8a3..59306bc 100644
>> --- a/gcc/doc/invoke.texi
>> +++ b/gcc/doc/invoke.texi
>> @@ -4032,14 +4032,41 @@ arguments in the C++ frontend.
>>  @item -fdiagnostics-urls[=@var{WHEN}]
>>  @opindex fdiagnostics-urls
>>  @cindex urls
>> +@vindex GCC_URLS @r{environment variable}
>> +@vindex TERM_URLS @r{environment variable}
>>  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".
> 
> We should avoid literal quotes in the manual instead of proper markup. Maybe @samp{dumb}?  Or is this trying to express something other than a literal string?
> 

okay, in this case, maybe it gets clearer when I write:

"standard error is a terminal, and when not executing in an emacs shell."

I moved the "dumb" to a comment in the C-function doing that detection
now, and consider it an implementation detail for the emacs detection.


>> +
>> +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 and non-empty in the environment of the compiler, or @samp{never}
>> +if neither are.
>> +
>> +However, even with @option{-fdiagnostics-urls=always} the behavior will be
> 
> s/will be/is/, unless this is something that hasn't been implemented yet.  :-S
> 

okay, thanks.

>> +dependent on those environment variables:
>> +If @env{GCC_URLS} set to empty or "no", do not embed URLs in diagnostics.
> 
> s/set/is set/ ??
> 

yes.

> And please avoid literal quotes in the text here too.
> 

done.

>> +If set to "st", URLs use ST escape sequences.
>> +If set to "bel", the default, URLs use BEL escape sequences.
> 
> I have no idea what ST and BEL escape sequences are....
> 

those are the acronyms of certain escape sequences, I added an explanation.

>> +Any other non-empty value enables the feature.
>> +If @env{GCC_URLS} is not set, use @env{TERM_URLS} as a fallback.
>> +
>> +At this time GCC tries to detect also a few terminals that are known to
>> +not implement the URL feature, and have an installed basis where the URL
> 
> What is "an installed basis"?
> 

I meant that old versions are still installed on a lot of places,
and we do not want to break them.
I re-worded that paragraph, to be hopefully clearer now.

>> +escapes are likely to mis-behave, i.e. print garbage on the screen.
> 
> s/mis-behave/misbehave/
> 

done.

>> +That list is currently xfce4-terminal and tmux and mingw, this can be
>> +overridden with the @option{-fdiagnostics-urls=always}.
> 
> Are those things literal strings for something?  How do you use that option to override the list of terminals?
> 

No, xfce4-terminal and tmux are names of software packages.
mingw, is a minimal interface library, that allows GCC to run under Windows.
Windows has a command shell, that is printing garbage on windows 7,
when URL escapes are used and ignores the URLs on windows 10, but
there is also a windows terminal program that prints garbage in all versions.
Therefore this feature cannot be used on windows, but it is known to cause
screen corruptions at least on some Windows versions, that are still in use.

> Frankly, this whole feature and the options that control it seem horribly over-complex to me.  :-S
> 

Thanks for your review,
I attached a changes.diff with just the changes in response to your review,
and a complete git format-patch file, containing the latest patch.


Is it OK for trunk?

Thanks
Bernd.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: changes.diff
Type: text/x-patch
Size: 5093 bytes
Desc: changes.diff
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20200202/bf7a1ff5/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-PR-87488-Add-with-diagnostics-urls-configuration-opt.patch
Type: text/x-patch
Size: 19278 bytes
Desc:  0001-PR-87488-Add-with-diagnostics-urls-configuration-opt.patch
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20200202/bf7a1ff5/attachment-0001.bin>


More information about the Gcc-patches mailing list