On Tue, 2023-11-07 at 08:04 +0100, Simon Sobisch wrote:
> Thank you very much for this proof-of-concept use!
>
> Inspecting it raises the following questions to me, both for a
> possible
> binutils implementation and for the library use in general:
>
> * How should the application set the relevant context (often lines
> are
> shown before/after)?
Currently the source-printing code has this logic (in gcc/diagnostic-
show-locus.cc):
- filter locations within the diagnostic to "sufficiently sane" ones
(thus ignoring e.g. ranges that end before they start)
- look at all of the remaining locations that are in the same source
file as the primary location of the diagnostic
- determine a set of runs of source lines; layout::calculate_line_spans
has this comment:
/* We want to print the pertinent source code at a diagnostic. The
rich_location can contain multiple locations. This will have been
filtered into m_exploc (the caret for the primary location) and
m_layout_ranges, for those ranges within the same source file.
We will print a subset of the lines within the source file in question,
as a collection of "spans" of lines.
This function populates m_line_spans with an ordered, disjoint list of
the line spans of interest.
Printing a gap between line spans takes one line, so, when printing
line numbers, we allow a gap of up to one line between spans when
merging, since it makes more sense to print the source line rather than a
"gap-in-line-numbering" line. When not printing line numbers, it's
better to be more explicit about what's going on, so keeping them as
separate spans is preferred.
For example, if the primary range is on lines 8-10, with secondary ranges
covering lines 5-6 and lines 13-15:
004
005 |RANGE 1
006 |RANGE 1
007
008 |PRIMARY RANGE
009 |PRIMARY CARET
010 |PRIMARY RANGE
011
012
013 |RANGE 2
014 |RANGE 2
015 |RANGE 2
016
With line numbering on, we want two spans: lines 5-10 and lines 13-15.
With line numbering off (with span headers), we want three spans: lines 5-6,
lines 8-10, and lines 13-15. */
(end of quote)
This algorithm could be tweaked if you want extra lines of context,
perhaps having an integer option N for N extra lines of before/after
context around each run.
> * Should it be possible to override msgid used to display the
> warning/error type?
> If this would be possible then the text sink in messages_init may
> be
> adjusted to replace the label with _("Warning") and _("Error"),
> which
> would leave the text output "as-is" (if the text sink is
> configured to
> not output the source line); this would make it usable without
> adjusting the testsuite and to adopt to a standard later.
For the msgids, I was more thinking of the messages of the diagnostics
themselves; for instance, in GCC the error format string:
"Invalid argument %d for builtin %qF"
has fr.po translation:
"Argument %d invalide pour la fonction interne %qF"
But it sounds like gas also has capitalized severities (e.g.
"Warning"), so maybe that should simply be a flag in the text sink.
>
>
> Notes for the SARIF output:
> * the region contains an error, according to the linked JSON spec
> startColumn has a minimum of 1 (I guess you'd just leave it away
> if
> the application did not set it)
Good catch; looks like a bug in my SARIF output code. I've filed it
for myself as:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112425
> * the application should have the option to pre-set the
> sourceLanguage
> for the diagnostic_manager (maybe even make that a positional
> argument
> that needs to be passed but can be NULL) and override it when
> specifying a region
Why?
Note that the sourceLanguage can always be NULL. I considered setting
it for gas, but it's not clear what the value can be, so I just omit
it; see:
https://github.com/oasis-tcs/sarif-spec/issues/608
[..snip...]
Thanks for the feedback
Dave
