[PATCH] D50154: [clangd] capitalize diagnostic messages

[Alex L via cfe-commits]

On Tue, 7 Aug 2018 at 11:38, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Tue, Aug 7, 2018 at 11:22 AM Alex L <arphaman at gmail.com> wrote:
>
>> On Tue, 7 Aug 2018 at 10:52, David Blaikie via cfe-commits <
>> cfe-commits at lists.llvm.org> wrote:
>>
>>>
>>>
>>> On Tue, Aug 7, 2018 at 10:33 AM Alex Lorenz via Phabricator <
>>> reviews at reviews.llvm.org> wrote:
>>>
>>>> arphaman added a comment.
>>>>
>>>> In https://reviews.llvm.org/D50154#1191002, @dblaikie wrote:
>>>>
>>>> > What's the motivation for clangd to differ from clang here? (& if the
>>>> first
>>>> >  letter is going to be capitalized, should there be a period at the
>>>> end? But
>>>> >  also the phrasing of most/all diagnostic text isn't in the form of
>>>> complete
>>>> >  sentences, so this might not make sense)
>>>>
>>>>
>>>> It's mostly for the presentation purposes, to match the needs of our
>>>> client. I first implemented it as an opt-in feature, but the consensus was
>>>> to capitalize the messages all the time.
>>>>
>>>
>>> Doesn't seem like it'd be any more expensive (amount of code or
>>> performance) to do that up in your client code, then, would it? I guess if
>>> most users of this API in time ended up preferring capitalized values, it'd
>>> make sense to share that implementation - but to me it seems like a strange
>>> transformation to happen at this level. (since it depends on what kind of
>>> client/how they want to render things - so it seems an odd choice to bake
>>> in to the API (or even provide an option for, unless there are lots of
>>> users/the code was especially complicated))
>>>
>>> My 2c - I've no vested interest or authority here.
>>>
>>
>> I think it's more in spirit with Clangd to provide output that's as close
>> to the one presented by the client as possible.
>>
>
> That assumes there's one client though, right? Different clients might
> reasonably have different needs for how they'd want the text rendered, I'd
> imagine.
>

True. This transformation is lossless though, so the clients will still be
able to get back the original text if needed. And if the consensus about
this particular text transformation changes I'm willing to revert this
change for sure.


>
>
>> I would argue there's already a precedence for this kind of
>> transformations, for example, Clangd merges the diagnostic messages of
>> notes and the main diagnostics into one, to make it a better presentation
>> experience in the client:
>>
>>
>> https://github.com/llvm-mirror/clang-tools-extra/blob/55bfabcc1bd75447d6338ffe6ff27c1624a8c15a/clangd/Diagnostics.cpp#L161
>>
>
> I'm assuming that's because the API/protocol only supports a single
> message, so the multi-message/location nuance of LLVM's diagnostics are
> necessarily lost when converting to that format?
>
> If that's not the case, and the API/protocol does support a
> multiple-message diagnostic & ClangD is collapsing them early - that also
> seems like an unfortunate loss in fidelity.
>

Clangd sends out both the main diagnostic, and the attached notes (that
don't have fixits) in a list (i.e. the hierarchy isn't directly preserved,
although it can be recreated by the client).
So it looks like they're collapsed early, but at the same time the client
has enough information to do this transformation itself if desired.
I'm planning to work on an option to remove this behavior if desired by the
client.


>
>
>>
>>
>>
>>
>>
>>>
>>>
>>>> I don't think it would make sense to insert the period at the end,
>>>> because, as you said, not all diagnostics are complete sentences
>>>>
>>>>
>>>> Repository:
>>>>   rCTE Clang Tools Extra
>>>>
>>>> https://reviews.llvm.org/D50154
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180807/5c31abb2/attachment.html>