[PATCH] D145284: WIP [clang] adds capabilities for SARIF to be written to file

[Christopher Di Bella via Phabricator via llvm-commits]

cjdb marked an inline comment as done.
cjdb added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticOptions.h:110
+  /// The file to serialise text diagnostics to (non-appending).
+  std::string FilePath;
+
----------------
erichkeane wrote:
> I'm a touch disturbed by there being 3 very similar file names with very similar comments here. Is there a reason we need all 3?
Agreed. I tried to get this to work with `-serialize-diagnostics` but that seems to activate a path that causes a different diagnostic engine to take over based on the value of `DiagnosticSerializationFile`. I didn't notice `DiagnosticLogFile`, so I'll see what happens if that is used.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4036
+    CmdArgs.push_back(Format->getValue());
+    if (StringRef(Format->getValue()).starts_with("sarif"))
       D.Diag(diag::warn_drv_sarif_format_unstable);
----------------
erichkeane wrote:
> are we intentionally getting rid of the SARIF spellings intentionally?  If so, and because we have this warning, should we report they are no longer supported?
Yes. SARIF support isn't documented at all, and as such, I suspect the number of people who know about it (and use it for more than simple curiosity) would be hand-countable, so I'm not sure that it's worth the effort?

(The reason it's not documented is because I forgot to ask for that last year, but that's currently to our advantage.)


================
Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:37
+  return std::make_unique<llvm::raw_fd_ostream>(
+      std::string(TargetPath) + ".sarif", EC);
+}
----------------
erichkeane wrote:
> Isn't TargetPath a 'Path'?  If so, are you just creating a dot-file?  Aren't those really nasty on Windows, and not a nice thing to do here on Linux?
`TargetPath` should be a filename, not a directory. When implicit, this is whatever the compiler will generate as its target program. When explicit... I should probably add a check and diagnostic for this.


================
Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:45
+  if (*EC) {
+    assert(false and "not implemented yet: not even sure what to do");
+  }
----------------
erichkeane wrote:
> I know these are nice/readable/etc, but llvm doesn't use these.
> 
> Also, you KNOW what EC equals here: it is a default constructed `std::error_code`, so the 'if' condition here is always hit.  I suspect just removing the whole body and replacing with an `llvm_unreachable` is appropriate instead.
I'm not sure this code is unreachable, hence the assert message. If the file can't be opened for some reason, it's not appropriate for Clang to simply crash.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145284