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

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added inline comments.


================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:155
+    break;
+  }
+}
----------------
Does this need an `llvm_unreachable` after the switch?


================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:177
+      // original path as possible, because that helps a user to recognize it.
+      // real_path() expands all links, which sometimes too much. Luckily,
+      // on Windows we can just use llvm::sys::path::remove_dots(), because,
----------------



================
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(
----------------
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 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131632