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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
