This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Implement hover for string literals
ClosedPublic

Authored by v1nh1shungry on Nov 8 2022, 8:42 AM.

Details

Summary

Show string-literals' type and size in a hover card

Issue related: https://github.com/clangd/clangd/issues/1016

Diff Detail

Event Timeline

v1nh1shungry created this revision.Nov 8 2022, 8:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2022, 8:42 AM
v1nh1shungry requested review of this revision.Nov 8 2022, 8:42 AM
tom-anders added inline comments.Nov 8 2022, 11:01 AM
clang-tools-extra/clangd/Hover.cpp
785

Is it really useful to show the contents inside the Hover? You already see the contents of the string literal anyway, so I don't think this adds any value

788

(nit)

814

Hmm so what's stopping us from adding support for C here?

820

I think you should prefer llvm::formatv here

@tom-anders Thanks for reviewing and for your suggestions!

clang-tools-extra/clangd/Hover.cpp
785

I added this comment only because others show the contents. I think you're right.

814

Because I'm not that familiar with C's data types and standards.

Improve the patch

Support different C++ standards

Improve the patch

Seems like I have made things too complex :(

Should work for C, C++, and other languages now.

v1nh1shungry retitled this revision from [clangd] Implement hover for C++ string literals to [clangd] Implement hover for string literals.Nov 8 2022, 9:35 PM
v1nh1shungry edited the summary of this revision. (Show Details)

Improve the patch

kadircet added inline comments.Nov 9 2022, 8:13 AM
clang-tools-extra/clangd/Hover.cpp
868

can you extract this into a function, similar to the cases below?

871

i think it's a good idea to also include the size here (e.g. L"XYZ", would be 8 bytes).

Apply the reviewing suggestions

@kadircet Thanks for your suggestions!

v1nh1shungry edited the summary of this revision. (Show Details)Nov 9 2022, 7:33 PM

Modify the name in the hover info

kadircet accepted this revision.Nov 10 2022, 1:17 AM

thanks, lgtm!

This revision is now accepted and ready to land.Nov 10 2022, 1:17 AM

@kadircet Could you please help me commit it? Thanks a lot!

sure i am happy to land, @tom-anders do you have anything else to add ?

sure i am happy to land, @tom-anders do you have anything else to add ?

LGTM!

This revision was automatically updated to reflect the committed changes.