denik added inline comments.
================
Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:71
+ // other infrastructure necessary when emitting more rich diagnostics.
+ if (!Info.getLocation().isValid()) { // TODO: What is this case?
+ // SARIFDiag->addDiagnosticWithoutLocation(
----------------
cjdb wrote:
> aaron.ballman wrote:
> > vaibhav.y wrote:
> > > vaibhav.y wrote:
> > > > The location maybe if the diagnostic's source is located in the scratch
> > > > buffer. Likely for macro expansions where token pasting is involved.
> > > > Another case would be errors on the command line.
> > > >
> > > > I'm not entirely sure how the SARIF spec would handle this case, it
> > > > might require an extension.
> > > >
> > > > A few ways that might work could be:
> > > >
> > > > Using the [[
> > > > https://docs.oasis-open.org/sarif/sarif/v2.0/csprd02/sarif-v2.0-csprd02.html#_Toc10127692
> > > > | logicalLocations ]] property to specify ([[
> > > > https://docs.oasis-open.org/sarif/sarif/v2.0/csprd02/sarif-v2.0-csprd02.html#_Toc10127910
> > > > | logicalLocation object ]]), this might need an extension for kind:
> > > > "macro", another case that might need extension is diagnostics about
> > > > invalid command line flags which are also diagnostics without a valid
> > > >
> > > > The parentIndex for these logical locations could be set to the
> > > > physical location that produced them.
> > > >
> > > > I think this definitely warrants some discussion since the spec doesn't
> > > > provide a clear way to express these cases.
> > > >
> > > > WDYT @aaron.ballman @cjdb @denik
> > > The spec does say for "kind":
> > >
> > > > If none of those strings accurately describes the construct, kind MAY
> > > > contain any value specified by the analysis tool.
> > >
> > > So an extension might not be necessary, but might be worth discussing.
> > From looking through the spec, I think `logicalLocations` is probably the
> > right choice and we'd want to make up our own kind for things like the
> > scratch buffer or the command line. I think an extension would be worth
> > discussing.
> We should defer this to a future CL, so that Abraham isn't blocked by our
> decision-making (and so we can make the right decision). I can start a GitHub
> issue to get the discussion in a good spot?
I agree that `logicalLocations` is a good use for the mentioned cases.
In addition, the spec mentions that `location object` can miss both physical
and logical location properties in which cases it has to include the `message`
property (see [[
https://docs.oasis-open.org/sarif/sarif/v2.0/csprd02/sarif-v2.0-csprd02.html#_Toc10127861
| 3.28.1 ]]). I think this also could be a good use case for the command line
flag errors.
To unblock this change we can leave `result.locations` empty. It's allowed by
the spec (see [[
https://docs.oasis-open.org/sarif/sarif/v2.0/csprd02/sarif-v2.0-csprd02.html#_Toc10127841
| 3.27.12 ]]). From what I see, `Sarif.cpp` doesn't use the source manager if
`Locations` is empty.
But `locations` still has to be present with the empty list. This might need a
fix in https://clang.llvm.org/doxygen/Sarif_8cpp_source.html#l00387.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131632/new/
https://reviews.llvm.org/D131632
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
