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

[Christopher Di Bella via Phabricator via libcxx-commits]

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