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