cjdb added a comment. In D138939#3964677 <https://reviews.llvm.org/D138939#3964677>, @erichkeane wrote:
> In D138939#3964439 <https://reviews.llvm.org/D138939#3964439>, @erichkeane > wrote: > >> In D138939#3964404 <https://reviews.llvm.org/D138939#3964404>, @cjdb wrote: >> >>> In D138939#3963496 <https://reviews.llvm.org/D138939#3963496>, @erichkeane >>> wrote: >>> >>>> In D138939#3963473 <https://reviews.llvm.org/D138939#3963473>, @tschuett >>>> wrote: >>>> >>>>> Maybe `void FormatDiagnostic(SmallVectorImpl<char> &OutStr, >>>>> DiagnosticMode mode)`instead of `void >>>>> FormatDiagnostic(SmallVectorImpl<char> &OutStr)`? >>>>> To make the transition easer and future proof. >>>> >>>> I like this idea. Perhaps with DiagnosticMode being a 3-way enum: >>>> >>>> enum struct DiagnosticMode { >>>> Legacy, >>>> Sarif, >>>> Default = Legacy >>>> } >>>> >>>> I like the idea in particular, since it makes a command line flag to >>>> modify "Default" to be whichever the user prefers pretty trivial. >>> >>> There's already a flag for this: `-fdiagnostics-format=sarif`. Why do we >>> need a second diagnostic mode flag? >> >> Ah, oh... is the Sarif formatting being done with a new formatter? That >> seems unfortunate, since folks using the other formatters won't be able to >> use the user friendly formats. > > I've been alerted offline that I am misunderstanding the Sarif proposal, and > where this is going. I'll note that I wasn't present/invited at the calls > where all of this was discussed, so I am admittedly not completely up to > date. The above concern shouldn't stop others from reviewing this, > particularly if you better understand the intent here. I don't necessarily think you're the one misunderstanding here. We're definitely talking past one another, but I think it's equally likely that you're asking for something reasonable, and I'm mixing it up with something else. ================ Comment at: clang/include/clang/Frontend/DiagnosticRenderer.h:130 /// \param Message The diagnostic message to emit. + /// \param Reason Supplementary information for the message. /// \param Ranges The underlined ranges for this code snippet. ---------------- rymiel wrote: > Which parameter is this doxygen comment referring to? Thanks, that was from an old iteration of the CL. ================ 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 ---------------- erichkeane wrote: > Comment no longer matches. I've deleted the comment because it's already in the header, and risks going stale over and over. ================ 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'}} ---------------- erichkeane wrote: > 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. This is maybe done? I'm not sure if this is the #BOOKMARK style you're referring to, but it should capture the same intent. Lemme know if you had something else in mind and I'll happily change it 🙂 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
