This is an archive of the discontinued LLVM Phabricator instance.

[clangd] show layout info when hovering on a class/field definition.
ClosedPublic

Authored by sammccall on Apr 2 2020, 6:07 PM.

Details

Summary

This triggers only on the definition itself, not on references (probably too
noisy). Inspecting the definition seems like a decent hint for being interested
in layout.

Diff Detail

Event Timeline

sammccall created this revision.Apr 2 2020, 6:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2020, 6:07 PM
kadircet added inline comments.Apr 3 2020, 1:51 AM
clang-tools-extra/clangd/Hover.cpp
659

QuantityType seems to be an alias for int64_t, not sure how likely it is that someone will have a type that's bigger than 4GB, but still, should we make typeof HoverInfo::Size optional<int64_t> instead of unsigned?

661

nit: either make this else if or put a return above ?

664

similar to size, offset is also of type uint64_t

clang-tools-extra/clangd/unittests/HoverTests.cpp
69

any reason for changing these from int to char ?

1882

maybe also add offset?

sammccall updated this revision to Diff 254786.Apr 3 2020, 6:56 AM
sammccall marked 5 inline comments as done.

address comments

clang-tools-extra/clangd/unittests/HoverTests.cpp
69

hardcoding sizeof(int) isn't portable unless we want to set the target explicitly

kadircet accepted this revision.Apr 3 2020, 7:13 AM
kadircet marked an inline comment as done.

thanks!

clang-tools-extra/clangd/unittests/HoverTests.cpp
69

right, the test already sets the target though.

I suppose relying less on it is better, so feel free to keep it.

This revision is now accepted and ready to land.Apr 3 2020, 7:13 AM
nridge added a subscriber: nridge.Apr 4 2020, 4:58 PM
This revision was automatically updated to reflect the committed changes.