[PATCH] D50154: [clangd] capitalize diagnostic messages

[David Blaikie via cfe-commits]

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.


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


>
>
>
>
>
>>
>>
>>> 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/a48c554b/attachment.html>