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

[Abraham Corea Diaz via Phabricator via cfe-commits]

abrahamcd added inline comments.


================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:75
+            emitFilename(FE->getName(), Loc.getManager());
+        // FIXME: No current way to add file-only location to SARIF object
+      }
----------------
@vaibhav.y , just wanted to confirm, the SarifDiagnosticWriter can only add locations that include row/column numbers, correct?


================
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(
----------------
Does anyone know specific kinds of diagnostics that might be covered here in traditional diagnostics? I'm having trouble thinking of how we can support a simpler path for these kinds of diagnostics in SARIF since the Writer requires a source manager.


================
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"}
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131632/new/

https://reviews.llvm.org/D131632