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

[Aaron Ballman via Phabricator via cfe-commits]

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