[PATCH] D50154: [clangd] capitalize diagnostic messages

Tue Aug 7 16:21:54 PDT 2018

On Tue, Aug 7, 2018 at 4:02 PM Alex L <arphaman at gmail.com> wrote:

> 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.
>

I'm not sure that c == lower(upper(c)) - well, if the character is ever
uppercase to begin with, it's clearly not. But even in the case of a
lowercase character to start with I didn't think that was always true - I
guess for ASCII/English it is, and that's all we're dealing with here.

> And if the consensus about this particular text transformation changes I'm
> willing to revert this change for sure.
>

*nod* I mean if everyone who's invested in/working on clangd agrees with
your direction, that's cool/good. My 2c is that this sort of thing seems
like the responsibility of the client, not the API - but that's by no means
authoritative.

> 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.
>

*nod* I'd (completely in the abstract - not having worked on clangd or its
clients, so take with grain of salt, etc) probably still leave this up to
the client (which would be easier if the client were an in-process API kind
of thing rather than a strict protocol - because it'd be easy to include a
function clients could call to collapse warnings+notes if they wanted to
without needing to change the base behavior). Though this case is
admittedly a fraction more nuanced rather than uppercasing one letter - bit
more code to concatenate things together, etc. (though perhaps then all the
more reason that it's likely that different clients might have different
concatenation needs/formatting preferences/etc)

>
>
>>
>>
>>>
>>>
>>>
>>>
>>>
>>>>
>>>>
>>>>> 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/95ae6c86/attachment-0001.html>