abrahamcd added inline comments.
================
Comment at: clang/include/clang/Frontend/SARIFDiagnosticPrinter.h:63-65
+ void setSarifWriter(SarifDocumentWriter *SarifWriter) {
+ Writer = std::unique_ptr<SarifDocumentWriter>(SarifWriter);
+ }
----------------
aaron.ballman wrote:
> This interface seems dangerous to me -- the caller has no idea that the
> pointer will be stashed off as a `unique_ptr` and so the caller might expect
> to still be able to use its (now not unique) pointer value.
Would it be best to store as a regular pointer instead of a unique pointer in
this case then?
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:162
+ break;
+ llvm_unreachable("Not an existing diagnostic level");
+ }
----------------
cjdb wrote:
> aaron.ballman wrote:
> >
>
This creates `-Wcovered-switch-default` warning so I just removed the
unreachable.
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:174
+ if (DiagOpts->AbsolutePath) {
+ auto File = SM.getFileManager().getFile(Filename);
+ if (File) {
----------------
aaron.ballman wrote:
> Please spell out the type since it's not spelled out in the initialization.
> Also, the next line is checking for a file -- what if `File` is null?
If I am interpreting it correctly (this is adapted from `TextDiagnostic.cpp`),
I believe the passed-in Filename will just be returned unaltered. This is
probably one of those functions that would be good to pull out during
refactoring.
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:189
+ // on Windows we can just use llvm::sys::path::remove_dots(), because,
+ // on that system, both aforementioned paths point to the same place.
+#ifdef _WIN32
----------------
aaron.ballman wrote:
> I'm not certain we want a difference in behavior here. For starters, Windows
> also has hardlinks and symlinks (they're not exactly the same as on *nix
> systems though), so I think the assumption these paths point to the same
> place is valid. But also, platform-specific behavior should be exceedingly
> rare in most parts of the compiler. Is there significant harm in unifying
> the behavior across platforms?
There seems to be Windows-specific tests that rely on this behavior, but I'm
not too sure what the harm in unifying it would be. There's some discussion on
it here: D59415.
================
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"}
+
----------------
cjdb wrote:
> 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?
Yes, I meant "this FileCheck implementation" (back then I had the unit test
implementation in here as well but I have since removed it).
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
