[PATCH] D151128: [clangd] Show size, offset and padding for bit fields on hover

[Sam McCall via Phabricator via cfe-commits]

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