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