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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
