[PATCH] D50154: [clangd] capitalize diagnostic messages
Fangrui Song via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 7 17:40:39 PDT 2018
On 2018-08-07, David Blaikie wrote:
>
>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.
Not bijection :) classical German example 'ß'.upper().lower() => 'ss'
Though for the context (diagnostic messages where i18n is not
concerned), this works well enough.
>
>
> 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.
Alex, would you mind sharing the discussion thread of the editor you use?
>
> 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)
For this particular example (mainMessage), I guess it is done this way
(and also reasonable to do this on the server side) because in the LSP
specification, `message` has the type `string` (instead of an arbitrary
`object`)
https://microsoft.github.io/language-server-protocol/specification#diagnostic
> 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
>
--
宋方睿
More information about the cfe-commits
mailing list