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

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added inline comments.


================
Comment at: clang/lib/Frontend/FrontendAction.cpp:727-728
+      static_cast<SARIFDiagnosticPrinter *>(&CI.getDiagnosticClient())
+          ->setSarifWriter(std::unique_ptr<SarifDocumentWriter>(
+              new SarifDocumentWriter(CI.getSourceManager())));
+    }
----------------
Now that the interface is using `unique_ptr`, we should replace with `std::make_unique`.


================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:129
+
+  // FIXME: Enable the proper representation of PresumedLocs modified by a 
+  // #line directive after relevant change is made in SarifDocumentWriter.
----------------
abrahamcd wrote:
> 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?
Please assign this a GitHub issue. You can assign the issue to me.


================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:191
+#ifdef _WIN32
+      TmpFilename = (*File)->getName();
+      llvm::sys::fs::make_absolute(TmpFilename);
----------------
aaron.ballman wrote:
> Note: this is not a particularly small string when it requires 4k by default.
There's a bug either here or in the function's interface: the function returns a `StringRef` to a stack object. Do we really need ownership here?


================
Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:35-36
+SARIFDiagnosticPrinter::~SARIFDiagnosticPrinter() {
+  if (OwnsOutputStream)
+    delete &OS;
+}
----------------
This makes me very uncomfortable. @aaron.ballman do we have a smart-pointer that works like `variant<T*, unique_ptr<T>>`?


================
Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:43
+  assert(hasSarifWriter() && "Writer not set!");
+  SARIFDiag.reset(new SARIFDiagnostic(*OS, LO, &*DiagOpts, &*Writer));
+  // Initialize the SARIF object.
----------------
Please replace with `make_unique`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131632