[PATCH] D131632: [clang] Enable output of SARIF diagnostics

Mon Aug 22 16:56:16 PDT 2022

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