sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Sorry for the delay, I've been out on vacation.
Behavior looks great and isn't too complicated :-)
Let me know if you have commit access or I should land this for you (in which
case: please provide a preferred name/email for attribution)
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1467
- if (Offset)
- Output.addParagraph().appendText(
- llvm::formatv("Offset: {0} byte{1}", *Offset, *Offset == 1 ? "" : "s")
- .str());
+ const auto FormatSize = [](uint64_t Bits) {
+ uint64_t Value = Bits % 8 == 0 ? Bits / 8 : Bits;
----------------
can you make this a separate (static) function rather than a lambda?
and the same for `formatOffset()`
(This is a long function as it is)
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1474
+
+ if (Offset) {
+ const auto Bytes = *Offset / 8;
----------------
The distinction between bits+bytes for offsets vs bits OR bytes for size is
worth calling out with a comment:
```
// Sizes (and padding) are shown in bytes if possible, otherwise in bits.
string formatSize(...) { ... }
// Offsets are shown in bytes + bits, so offsets of different fields
// can always be easily compared.
string formatOffset(...) { ... }
```
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1478
+ auto &P = Output.addParagraph().appendText("Offset: ");
+ if (Bytes != 0 || Bits == 0)
+ P.appendText(FormatSize(Bytes * 8));
----------------
FWIW I think I slightly prefer printing bytes always, so that 4 prints as `0
bytes and 4 bits`.
This makes a rare case slightly more regular (and the code is simpler). Up to
you though.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3292
{
[](HoverInfo &HI) {
HI.Kind = index::SymbolKind::Field;
----------------
can you add a testcase for render() where the size/offset/padding are not whole
bytes?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151128/new/
https://reviews.llvm.org/D151128
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
