[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:116
+
+    Locations.push_back(
+        CharSourceRange{SourceRange{BeginLoc, EndLoc}, /* ITR = */ false});
----------------
I noticed that when processing additional source ranges, regular source locations are used instead of the presumed locations. Is this an intentional difference from single location diagnostics? (This issue is present in the current TextDiagnostics as well.)


================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:129-132
+  // FIXME: Enable the proper representation of PresumedLocs modified by a 
+  // #line directive after relevant change is made in SarifDocumentWriter.
+  Locations.push_back(
+      CharSourceRange{SourceRange{DiagLoc, DiagLoc}, /* ITR = */ false});
----------------
There's an issue I encountered with adding presumed locations that have been modified by a #line directive to SARIF. To add a location, the Writer requires a CharSourceRange, which uses regular SourceLocations. If the presumed location has a modified line number, the source manager will still interpret it as a regular line number in the source file and can cause the wrong column number to be added to the SARIF object.

@vaibhav.y Do you have an idea of how involved it would be to add an option to the document writer that adds locations without first creating a CharSourceRange?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131632