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

[Erich Keane via Phabricator via llvm-commits]

erichkeane added a subscriber: jansvoboda11.
erichkeane added a comment.

Patch topic needs "WIP" out of it I think?

Just some WIP comments, I don't believe I'd be the one to approve this anyway.



================
Comment at: clang/include/clang/Basic/DiagnosticOptions.h:109
 
+  /// The file to serialise text diagnostics to (non-appending).
+  std::string FilePath;
----------------
typically Clang uses the 'American' spelling of things.


================
Comment at: clang/include/clang/Basic/DiagnosticOptions.h:110
+  /// The file to serialise text diagnostics to (non-appending).
+  std::string FilePath;
+
----------------
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?


================
Comment at: clang/include/clang/Driver/Options.td:5851
+def fdiagnostics_file_path : Separate<["-"], "fdiagnostics-file-path">,
+  HelpText<"FIX BEFORE MERGING">,
+  MarshallingInfoString<DiagnosticOpts<"FilePath">>;
----------------
Another 'TODO' here?


================
Comment at: clang/include/clang/Frontend/CompilerInstance.h:610
+  ///
+  /// \param ToFile Determines if Clang should write diagnostics to a file.
   void createDiagnostics(DiagnosticConsumer *Client = nullptr,
----------------
This comment seems inaccurate.


================
Comment at: clang/include/clang/Frontend/SARIFDiagnosticPrinter.h:39
+  /// name suffixed with `.sarif`.
+  SARIFDiagnosticPrinter(StringRef TargetPath, DiagnosticOptions *Diags);
+
----------------
What do we do with the DiagnosticsOptions here?  Does this have to be a pointer, instead of a ref?

EDIT: I see the rest of the printers seem to do this, but I don't know if we ever have a case where these are null.  I guess having them consistent here is important, but if the next person came through and made these all 'refs', it would be nice :)


================
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);
----------------
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?


================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:357
+    if (OutputPath.empty())
+      goto Default;
+    Diags->setClient(new SARIFDiagnosticPrinter(OutputPath, Opts));
----------------
Oh my... I'll leave it to the Clang code owner/driver code owner (@jansvoboda11) to decide what they think of this.


================
Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:37
+  return std::make_unique<llvm::raw_fd_ostream>(
+      std::string(TargetPath) + ".sarif", EC);
+}
----------------
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?


================
Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:45
+  if (*EC) {
+    assert(false and "not implemented yet: not even sure what to do");
+  }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145284