erichkeane added a comment.
Herald added subscribers: Michael137, JDevlieghere.
A few comments. I don't mind the approach, though I find myself wondering if
the "FormatDiagnostic" call should stay the same, and choose the legacy-reason
only when a sarif reason doesn't exist? Or for some level of command line
control?
The clang-side interface to this seems a touch clunky, and I fear it'll make
continuing its use/generalizing its use less likely.
================
Comment at: clang/include/clang/Basic/DiagnosticIDs.h:165
+struct DiagnosticReason {
+ std::string Legacy;
+ std::string SARIF;
----------------
"Reason" is a part of the diagnostic itself, pre-rendered, right? Since these
strings are literals, can this be an llvm::StringLiteral instead? (the
constexpr-stringref type)?
Else, generating these is going to cost us at startup time.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:4520
+ DiagReason<
+ /*Legacy:*/"invalid explicitly-specified argument for template parameter
%0",
+ /*SARIF:*/"we passed a %select{type|value|class template}1 as our
%ordinal2 "
----------------
Already kidna hate this format here. Is there any way we could make this be
something more like:
`DiagReason<Legacy<"whatever">, SARIF<"whatever">>` ?
================
Comment at: clang/lib/Format/TokenAnalyzer.cpp:47
llvm::SmallVector<char, 128> Message;
- Info.FormatDiagnostic(Message);
+ Info.FormatSummary(Message);
+ Info.FormatLegacyReason(Message);
----------------
This pair of calls has happened ~20 times by the time I've scrolled here. Can
we do a single call here of some sort instead?
================
Comment at: clang/test/Frontend/sarif-reason.cpp:15
+void g() {
+ f1<0>(); // expected-error{{no matching function for call to 'f1'}}
+ f1<S>(); // expected-error{{no matching function for call to 'f1'}}
----------------
This is definitely a case where I'd love the diagnostics formatted/arranged
differently here. If you can use the #BOOKMARK style to make sure errors and
notes are together, it would better illustrate what you're trying to do here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138939/new/
https://reviews.llvm.org/D138939
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
