aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Frontend/SARIFDiagnosticPrinter.h:42
+
+ unsigned OwnsOutputStream : 1;
+
----------------
There's not a lot of benefit to using bit-fields here yet, so I'd make this
field a `bool` instead for the time being. If we add more fields here where
packing them into a bit-field helps, then we can revisit at that time.
================
Comment at: clang/include/clang/Frontend/SARIFDiagnosticPrinter.h:45
+public:
+ SARIFDiagnosticPrinter(raw_ostream &os, DiagnosticOptions *diags,
+ bool OwnsOutputStream = false);
----------------
================
Comment at: clang/include/clang/Frontend/SARIFDiagnosticPrinter.h:63-65
+ void setSarifWriter(SarifDocumentWriter *SarifWriter) {
+ Writer = std::unique_ptr<SarifDocumentWriter>(SarifWriter);
+ }
----------------
This interface seems dangerous to me -- the caller has no idea that the pointer
will be stashed off as a `unique_ptr` and so the caller might expect to still
be able to use its (now not unique) pointer value.
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:43
+
+ auto *Diag = D.dyn_cast<const Diagnostic *>();
+
----------------
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:43-47
+ auto *Diag = D.dyn_cast<const Diagnostic *>();
+
+ if (!Diag) {
+ return;
+ }
----------------
aaron.ballman wrote:
>
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:59-61
+ if (Loc.isValid()) {
+ Result = addLocationToResult(Result, Loc, PLoc, Ranges, *Diag);
+ }
----------------
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:90-92
+ if (!RI->isValid()) {
+ continue;
+ }
----------------
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:105-107
+ if (BInfo.first != CaretFileID || EInfo.first != CaretFileID) {
+ continue;
+ }
----------------
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:127
+ auto FID = PLoc.getFileID();
+ // Visual Studio 2010 or earlier expects column number to be off by one
+ unsigned int ColNo = (LangOpts.MSCompatibilityVersion &&
----------------
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:146
+ case DiagnosticsEngine::Ignored:
+ llvm_unreachable("Invalid diagnostic type");
+ case DiagnosticsEngine::Note:
----------------
This is reachable and it's a programmer mistake if we get here.
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:162-163
+ break;
+ llvm_unreachable("Not an existing diagnostic level");
+ }
+
----------------
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:169-173
+ const SourceManager &SM) {
+#ifdef _WIN32
+ SmallString<4096> TmpFilename;
+#endif
+ if (DiagOpts->AbsolutePath) {
----------------
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:174
+ if (DiagOpts->AbsolutePath) {
+ auto File = SM.getFileManager().getFile(Filename);
+ if (File) {
----------------
Please spell out the type since it's not spelled out in the initialization.
Also, the next line is checking for a file -- what if `File` is null?
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:189
+ // on Windows we can just use llvm::sys::path::remove_dots(), because,
+ // on that system, both aforementioned paths point to the same place.
+#ifdef _WIN32
----------------
I'm not certain we want a difference in behavior here. For starters, Windows
also has hardlinks and symlinks (they're not exactly the same as on *nix
systems though), so I think the assumption these paths point to the same place
is valid. But also, platform-specific behavior should be exceedingly rare in
most parts of the compiler. Is there significant harm in unifying the behavior
across platforms?
================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:191
+#ifdef _WIN32
+ TmpFilename = (*File)->getName();
+ llvm::sys::fs::make_absolute(TmpFilename);
----------------
Note: this is not a particularly small string when it requires 4k by default.
================
Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:27-30
+SARIFDiagnosticPrinter::SARIFDiagnosticPrinter(raw_ostream &os,
+ DiagnosticOptions *diags,
+ bool _OwnsOutputStream)
+ : OS(os), DiagOpts(diags), OwnsOutputStream(_OwnsOutputStream) {}
----------------
It's ugly but it's the pattern we use all over the codebase (and it corrects
the naming convention issues).
================
Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:56-57
+ const Diagnostic &Info) {
+ // Default implementation (Warnings/errors count). // Keeps track of the
+ // number of errors
+ DiagnosticConsumer::HandleDiagnostic(Level, Info);
----------------
================
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(
----------------
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
