[PATCH] D116786: [clangd] Add designator inlay hints for initializer lists.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D116786#3247800 <https://reviews.llvm.org/D116786#3247800>, @kadircet wrote:

> a drive by concern around possible flakiness of the interaction. have you checked how does it look like when the user is still forming the initializer? it might be annoying if their cursor kept jumping around while they're editing the (possibly half-formed) initializer.

Hmm, just tested and this is definitely a thing.
The biggest problem seems to be typing something that causes all prior designators to disappear.
This happens when you add an invalid expression to the init list, e.g a partial identifier you're typing.
(RecoveryExpr doesn't help here unless we can determine the type - dependent initlistexprs are not semantically analyzed).

Any ideas? :-(



================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:71
+    if (BasesI != BasesE)
+      return false; // Bases can't be designated. Should we make one up?
+    if (FieldsI != FieldsE) {
----------------
nridge wrote:
> We could consider using the type name of the base (but I also get the impression that aggregate initialization with bases is uncommon enough to not worry about right now)
Yeah, I think this is rare.
Given that, I'd rather not create the confusion by having these hints be almost-but-not-quite-always valid syntax.


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:81
+           // std::array<int,3> x = {1,2,3}. Designators not strictly valid!
+           (OneField && isReservedName(FieldName))))
+        return true;
----------------
nridge wrote:
> Should we try to more specifically restrict this to the case where the one field is an array? Otherwise skipping the field name feels a bit arbitrary.
I modeled this after `isIdiomaticBraceElisionEntity` in SemaInit.cpp, which doesn't have this restriction.
(Though I didn't bother to implement the "base only" case, and restricted it to fields with reserved names).

I can change it if you want, though note that this is only eliding reserved names, and we *fail* (refuse to produce a designator) for reserved names otherwise.


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:145
+
+    if (!Fields.append(Prefix, BraceElidedSubobject != nullptr))
+      continue; // no designator available for this subobject
----------------
nridge wrote:
> aside: `Prefix` on this line is a great use case for the `UsedAsMutableReference` modifier. Now, if only we could somehow integrate clangd's semantic highlighting into Phabricator...
Agree. I've also wondered about using `🌀` as an inlay-hint instead of a token modifier.


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:404
+    SourcePrefix = SourcePrefix.rtrim(IgnoreChars);
+    ParamName = ParamName.trim(IgnoreChars);
     // Other than that, the comment must contain exactly ParamName.
----------------
nridge wrote:
> Why do we need to trim `ParamName`?
We're just passing in the designator string here, it may be `.foo=` and I want to match `/*foo*/` and `/*foo=*/` as well as `/*.foo=*/`.

I'd consider adding `[]` here too, to allow `/*1=*/` to match `[1]=`.


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:660
+                        ExpectedHint{".x=", "x"}, ExpectedHint{".y=", "y"},
+                        ExpectedHint{"[0]=", "0"}, ExpectedHint{"[1]=", "1"});
+}
----------------
nridge wrote:
> I wonder how useful these array-index hints are.
> 
> Unlike the struct case, they're not taking information present "elsewhere" (field names from the structure declaration) and making it available in the initializer, they're just taking information already present in the initializer and making it more explicit.
> 
> On the other hand, I guess if it's important that you initialize the element at index 15 in particular to some value, these hints help ensure you don't make an off-by-one error...
> they're just taking information already present in the initializer and making it more explicit

This is usually true but not always, e.g.

```
struct Point { float x, y, z; };
Point shape[] = {
  /*[0].x=*/1.0,
  /*[0].y=*/0.3,
  /*[0].z=*/0.3,
  /*[1].x=*/0.3,
  /*[1].y=*/0.1,
  /*[1].z=*/0.6,
  ...
};
```

We could try some other behavior:
 - don't emit a hint if there's any array component
 - don't emit a hint if there's exactly one array component and nothing else
 - have array vs field designators separately configurable

I'm not sure how this will feel in practice. I quite like the idea of having them initially, but turning this category off by default to give a chance to dogfood and reevaluate


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116786/new/

https://reviews.llvm.org/D116786