cjdb added a comment.
This looks great, thank you so much @abrahamcd!
================
Comment at: clang/include/clang/Frontend/SARIFDiagnosticPrinter.h:31
+class SARIFDiagnosticPrinter : public DiagnosticConsumer {
+ raw_ostream &OS;
+ IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts;
----------------
Please make OS a pointer instead of a reference, since member references make
usage very unpleasant.
================
Comment at: clang/include/clang/Frontend/SARIFDiagnosticPrinter.h:31-42
+ raw_ostream &OS;
+ IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts;
+
+ /// Handle to the currently active text diagnostic emitter.
+ std::unique_ptr<SARIFDiagnostic> SARIFDiag;
+
+ /// A string to prefix to error messages.
----------------
cjdb wrote:
> Please make OS a pointer instead of a reference, since member references make
> usage very unpleasant.
Nit: please move all private members below the public interface, where possible.
================
Comment at: clang/include/clang/Frontend/SARIFDiagnosticPrinter.h:42
+
+ unsigned OwnsOutputStream : 1;
+
----------------
Can we make this a bool instead? Unless I'm mistaken, there's 31 bits of
padding for `OwnsOutputStream`, as opposed to a bool's (almost certainly) eight
bits.
================
Comment at: clang/include/clang/Frontend/SARIFDiagnosticPrinter.h:52
+ /// used.
+ void setPrefix(std::string Value) {
+ Prefix = std::move(Value);
----------------
We should be passing in a `StringRef` rather than a string, since this will
unnecessarily construct a string for C string literals.
================
Comment at: clang/lib/Frontend/FrontendAction.cpp:724-727
+ if (CI.getDiagnosticOpts().getFormat() == DiagnosticOptions::SARIF) {
+ auto *Writer = new SarifDocumentWriter(CI.getSourceManager());
+ static_cast<SARIFDiagnosticPrinter *>(&CI.getDiagnosticClient())
+ ->setSarifWriter(Writer);
----------------
We shouldn't have raw owning pointers, since they're vulnerable to memory
leaks. Even though LLVM doesn't throw exceptions, it's still good practice to
avoid them.
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:30-31
+#include <string>
+
+using namespace clang;
+
----------------
Please replace this using-directive with a namespace scope.
================
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
+ }
----------------
I think it would be good to file an issue on GitHub and change to
`FIXME(llvm-project/<issue>)`. @aaron.ballman WDYT?
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:86-88
+ for (ArrayRef<CharSourceRange>::const_iterator RI = Ranges.begin(),
+ RE = Ranges.end();
+ RI != RE; ++RI) {
----------------
I haven't seen any use of `RI` or `RE` in this loop, except to dereference
`RI`, so we should change this to a range-for loop.
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:162
+ break;
+ llvm_unreachable("Not an existing diagnostic level");
+ }
----------------
aaron.ballman wrote:
>
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:210-223
+/// ranges necessary.
+void SARIFDiagnostic::emitDiagnosticLoc(FullSourceLoc Loc, PresumedLoc PLoc,
+ DiagnosticsEngine::Level Level,
+ ArrayRef<CharSourceRange> Ranges) {}
+
+void SARIFDiagnostic::emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc)
{
+}
----------------
If these aren't called, then they should probably assert and say that they're
not implemented.
================
Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:24-25
+#include "llvm/Support/raw_ostream.h"
+#include <algorithm>
+using namespace clang;
+
----------------
Please replace with a namespace scope.
================
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(
----------------
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?
================
Comment at: clang/test/Frontend/sarif-diagnostics.cpp:3
+// RUN: FileCheck -dump-input=always %s --input-file=%t
+// CHECK:
{"$schema":"https://docs.oasis-open.org/sarif/sarif/v2.1.0/cos02/schemas/sarif-schema-2.1.0.json","runs":[{"artifacts":[{"length":2953,"location":{"index":0,"uri":"file:///usr/local/google/home/abrahamcd/projects/llvm-project/clang/test/Frontend/sarif-diagnostics.cpp"},"mimeType":"text/plain","roles":["resultFile"]}],"columnKind":"unicodeCodePoints","results":[{"locations":[{"physicalLocation":{"artifactLocation":{"index":0},"region":{"endColumn":1,"startColumn":1,"startLine":5}}}],"message":{"text":";'main'
must return
'int'"},"ruleId":"3463","ruleIndex":0},{"locations":[{"physicalLocation":{"artifactLocation":{"index":0},"region":{"endColumn":11,"startColumn":11,"startLine":6}}}],"message":{"text":"use
of undeclared identifier
'hello'"},"ruleId":"4601","ruleIndex":1},{"locations":[{"physicalLocation":{"artifactLocation":{"index":0},"region":{"endColumn":17,"startColumn":17,"startLine":8}}}],"message":{"text":"invalid
digit 'a' in decimal
constant"},"ruleId":"898","ruleIndex":2},{"locations":[{"physicalLocation":{"artifactLocation":{"index":0},"region":{"endColumn":5,"startColumn":5,"startLine":12}}}],"message":{"text":"misleading
indentation; statement is not part of the previous
'if'"},"ruleId":"1806","ruleIndex":3},{"locations":[{"physicalLocation":{"artifactLocation":{"index":0},"region":{"endColumn":3,"startColumn":3,"startLine":10}}}],"message":{"text":"previous
statement is
here"},"ruleId":"1730","ruleIndex":4},{"locations":[{"physicalLocation":{"artifactLocation":{"index":0},"region":{"endColumn":10,"startColumn":10,"startLine":11}}}],"message":{"text":"unused
variable
'Yes'"},"ruleId":"6536","ruleIndex":5},{"locations":[{"physicalLocation":{"artifactLocation":{"index":0},"region":{"endColumn":12,"startColumn":12,"startLine":14}}}],"message":{"text":"use
of undeclared identifier
'hi'"},"ruleId":"4601","ruleIndex":6},{"locations":[{"physicalLocation":{"artifactLocation":{"index":0},"region":{"endColumn":1,"startColumn":1,"startLine":16}}}],"message":{"text":"extraneous
closing brace
('}')"},"ruleId":"1399","ruleIndex":7}],"tool":{"driver":{"fullName":"","informationUri":"https://clang.llvm.org/docs/UsersManual.html","language":"en-US","name":"clang","rules":[{"fullDescription":{"text":""},"id":"3463","name":""},{"fullDescription":{"text":""},"id":"4601","name":""},{"fullDescription":{"text":""},"id":"898","name":""},{"fullDescription":{"text":""},"id":"1806","name":""},{"fullDescription":{"text":""},"id":"1730","name":""},{"fullDescription":{"text":""},"id":"6536","name":""},{"fullDescription":{"text":""},"id":"4601","name":""},{"fullDescription":{"text":""},"id":"1399","name":""}],"version":"16.0.0"}}}],"version":"2.1.0"}
+
----------------
abrahamcd wrote:
> In regards to testing this, my understanding is that we are moving away from
> the unit test GTest testing, correct? I included a FileCheck implementation
> as well, but I wasn't sure how effective just checking the whole SARIF object
> like this would be.
> In regards to testing this, my understanding is that we are moving away from
> the unit test GTest testing, correct?
Yep, that's correct.
> I included a FileCheck implementation as well, but I wasn't sure how
> effective just checking the whole SARIF object like this would be.
Isn't this the FileCheck test?
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
