aaron.ballman added inline comments.
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:78
+ emitFilename(FE->getName(), Loc.getManager());
+ // FIXME: No current way to add file-only location to SARIF object
+ }
----------------
cjdb wrote:
> I think it would be good to file an issue on GitHub and change to
> `FIXME(llvm-project/<issue>)`. @aaron.ballman WDYT?
I'd be fine with that.
================
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?
SGTM (I don't consider this to be blocking for this patch, FWIW).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131632/new/
https://reviews.llvm.org/D131632
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
