[libcxx-commits] [PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

[Erich Keane via Phabricator via libcxx-commits]

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