<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On 7 August 2018 at 17:40, Fangrui Song <span dir="ltr"><<a href="mailto:maskray@google.com" target="_blank">maskray@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
On 2018-08-07, David Blaikie wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On Tue, Aug 7, 2018 at 4:02 PM Alex L <<a href="mailto:arphaman@gmail.com" target="_blank">arphaman@gmail.com</a>> wrote:<br>
<br>
On Tue, 7 Aug 2018 at 11:38, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
<br>
On Tue, Aug 7, 2018 at 11:22 AM Alex L <<a href="mailto:arphaman@gmail.com" target="_blank">arphaman@gmail.com</a>> wrote:<br>
<br>
On Tue, 7 Aug 2018 at 10:52, David Blaikie via cfe-commits <<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
<br>
On Tue, Aug 7, 2018 at 10:33 AM Alex Lorenz via Phabricator <<br>
<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br>
<br>
arphaman added a comment.<br>
<br>
In <a href="https://reviews.llvm.org/D50154#1191002" rel="noreferrer" target="_blank">https://reviews.llvm.org/D5015<wbr>4#1191002</a>, @dblaikie<br>
wrote:<br>
<br>
> What's the motivation for clangd to differ from clang<br>
here? (& if the first<br>
> letter is going to be capitalized, should there be a<br>
period at the end? But<br>
> also the phrasing of most/all diagnostic text isn't in<br>
the form of complete<br>
> sentences, so this might not make sense)<br>
<br>
It's mostly for the presentation purposes, to match the<br>
needs of our client. I first implemented it as an opt-in<br>
feature, but the consensus was to capitalize the messages<br>
all the time.<br>
<br>
Doesn't seem like it'd be any more expensive (amount of code or<br>
performance) to do that up in your client code, then, would it?<br>
I guess if most users of this API in time ended up preferring<br>
capitalized values, it'd make sense to share that<br>
implementation - but to me it seems like a strange<br>
transformation to happen at this level. (since it depends on<br>
what kind of client/how they want to render things - so it<br>
seems an odd choice to bake in to the API (or even provide an<br>
option for, unless there are lots of users/the code was<br>
especially complicated))<br>
<br>
My 2c - I've no vested interest or authority here.<br>
<br>
I think it's more in spirit with Clangd to provide output that's as<br>
close to the one presented by the client as possible. <br>
<br>
That assumes there's one client though, right? Different clients might<br>
reasonably have different needs for how they'd want the text rendered,<br>
I'd imagine.<br>
<br>
True. This transformation is lossless though, so the clients will still be<br>
able to get back the original text if needed.<br>
<br>
I'm not sure that c == lower(upper(c)) - well, if the character is ever<br>
uppercase to begin with, it's clearly not. But even in the case of a lowercase<br>
character to start with I didn't think that was always true - I guess for ASCII<br>
/English it is, and that's all we're dealing with here.<br>
</blockquote>
<br></div></div>
Not bijection :) classical German example 'ß'.upper().lower() => 'ss'<br>
Though for the context (diagnostic messages where i18n is not<br>
concerned), this works well enough.<span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
And if the consensus about this particular text transformation changes I'm<br>
willing to revert this change for sure.<br>
<br>
*nod* I mean if everyone who's invested in/working on clangd agrees with your<br>
direction, that's cool/good. My 2c is that this sort of thing seems like the<br>
responsibility of the client, not the API - but that's by no means<br>
authoritative.<br>
</blockquote>
<br></span>
Alex, would you mind sharing the discussion thread of the editor you use?</blockquote><div><br></div><div>I'm not working with any particular editor right now, just working on supporting Clangd for an internal client that used libclang before. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I would argue there's already a precedence for this kind of<br>
transformations, for example, Clangd merges the diagnostic messages<br>
of notes and the main diagnostics into one, to make it a better<br>
presentation experience in the client:<br>
<br>
<a href="https://github.com/llvm-mirror/clang-tools-extra/blob/" rel="noreferrer" target="_blank">https://github.com/llvm-mirro<wbr>r/clang-tools-extra/blob/</a><br>
<wbr>55bfabcc1bd75447d6338ffe6ff27c<wbr>1624a8c15a/clangd/Diagnostics.<wbr>cpp#<br>
L161<br>
<br>
I'm assuming that's because the API/protocol only supports a single<br>
message, so the multi-message/location nuance of LLVM's diagnostics are<br>
necessarily lost when converting to that format?<br>
<br>
If that's not the case, and the API/protocol does support a<br>
multiple-message diagnostic & ClangD is collapsing them early - that<br>
also seems like an unfortunate loss in fidelity.<br>
<br>
Clangd sends out both the main diagnostic, and the attached notes (that<br>
don't have fixits) in a list (i.e. the hierarchy isn't directly preserved,<br>
although it can be recreated by the client).<br>
So it looks like they're collapsed early, but at the same time the client<br>
has enough information to do this transformation itself if desired.<br>
I'm planning to work on an option to remove this behavior if desired by the<br>
client.<br>
<br>
*nod* I'd (completely in the abstract - not having worked on clangd or its<br>
clients, so take with grain of salt, etc) probably still leave this up to the<br>
client (which would be easier if the client were an in-process API kind of<br>
thing rather than a strict protocol - because it'd be easy to include a<br>
function clients could call to collapse warnings+notes if they wanted to<br>
without needing to change the base behavior). Though this case is admittedly a<br>
fraction more nuanced rather than uppercasing one letter - bit more code to<br>
concatenate things together, etc. (though perhaps then all the more reason that<br>
it's likely that different clients might have different concatenation needs/<br>
formatting preferences/etc)<br>
</blockquote>
<br></div></div>
For this particular example (mainMessage), I guess it is done this way<br>
(and also reasonable to do this on the server side) because in the LSP<br>
specification, `message` has the type `string` (instead of an arbitrary<br>
`object`)<br>
<br>
<a href="https://microsoft.github.io/language-server-protocol/specification#diagnostic" rel="noreferrer" target="_blank">https://microsoft.github.io/la<wbr>nguage-server-protocol/specifi<wbr>cation#diagnostic</a><div class="HOEnZb"><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I don't think it would make sense to insert the period at<br>
the end, because, as you said, not all diagnostics are<br>
complete sentences<br>
<br>
Repository:<br>
rCTE Clang Tools Extra<br>
<br>
<a href="https://reviews.llvm.org/D50154" rel="noreferrer" target="_blank">https://reviews.llvm.org/D501<wbr>54</a><br>
<br>
_____________________________<wbr>__________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-<wbr>bin/mailman/listinfo/cfe-<wbr>commits</a><br>
<br>
</blockquote>
<br></div></div><span class="HOEnZb"><font color="#888888">
-- <br>
宋方睿<br>
</font></span></blockquote></div><br></div></div>