[PATCH] D50154: [clangd] capitalize diagnostic messages

Alex L via cfe-commits cfe-commits at lists.llvm.org
Tue Aug 7 18:43:36 PDT 2018


On 7 August 2018 at 17:40, Fangrui Song <maskray at google.com> wrote:

>
> 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'm not working with any particular editor right now, just working on
supporting Clangd for an internal client that used libclang before.


>
>
>
>>            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/specifi
> cation#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
>>
>>
> --
> 宋方睿
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180807/103853d9/attachment-0001.html>


More information about the cfe-commits mailing list