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

Thu Dec 1 06:11:39 PST 2022

erichkeane added a comment.

In D138939#3958185 <https://reviews.llvm.org/D138939#3958185>, @cjdb wrote:

>> The clang-side interface to this seems a touch clunky, and I fear it'll make continuing its use/generalizing its use less likely.
>
> Is this w.r.t. the `FormatDiagnostic` being split up, or is it something else? If it's the former: I'll be changing that to `FormatLegacyDiagnostic`, which //almost// gets us back to where we started.

Urgh, I was a bit afraid you'd ask that :D  It is more of a feeling I guess, which is perhaps biased by this patch being particularly in the diagnostics-handling code.  However, I suspect that over time, you're going to want to start switching all these uses of `FormatLegacyReason` over to `FormatSarifReason`, and I would want the 'easy way' to be the 'right' way in either case.  Having it be a 2-liner, or over-specifying what is otherwise the 'default' behavior is a bit disconcerting.

In D138939#3958231 <https://reviews.llvm.org/D138939#3958231>, @cjdb wrote:

>> 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?
>
> Hmm... isn't this the point of the diagnostic consumers?

I don't have a great feeling of that?  I haven't done much thinking into the diagnostics architecture over the years.  That said, the use of when we'd choose one vs the other isn't completely clear to me yet.

================
Comment at: clang/lib/Basic/Diagnostic.cpp:791

 /// FormatDiagnostic - Format this diagnostic into a string, substituting the
 /// formal arguments into the %0 slots.  The result is appended onto the Str
----------------
Comment no longer matches.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138939