On 2018-08-07, David Blaikie wrote:
On Tue, Aug 7, 2018 at 4:02 PM Alex L <arpha...@gmail.com> wrote:
On Tue, 7 Aug 2018 at 11:38, David Blaikie <dblai...@gmail.com> wrote:
On Tue, Aug 7, 2018 at 11:22 AM Alex L <arpha...@gmail.com> wrote:
On Tue, 7 Aug 2018 at 10:52, David Blaikie via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
On Tue, Aug 7, 2018 at 10:33 AM Alex Lorenz via Phabricator <
revi...@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@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
--
宋方睿
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
