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

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGce94432702bf: [clangd] Add designator inlay hints for 
initializer lists. (authored by sammccall).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116786

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -13,21 +13,22 @@
 #include "TestWorkspace.h"
 #include "XRefs.h"
 #include "support/Context.h"
+#include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 
-std::ostream <<(std::ostream , const InlayHint ) {
-  return Stream << Hint.label;
+llvm::raw_ostream <<(llvm::raw_ostream ,
+  const InlayHint ) {
+  return Stream << Hint.label << "@" << Hint.range;
 }
 
 namespace {
 
 using ::testing::ElementsAre;
 using ::testing::IsEmpty;
-using ::testing::UnorderedElementsAre;
 
 std::vector hintsOfKind(ParsedAST , InlayHintKind Kind) {
   std::vector Result;
@@ -45,17 +46,24 @@
   std::string RangeName;
   HintSide Side = Left;
 
-  friend std::ostream <<(std::ostream ,
-  const ExpectedHint ) {
-return Stream << Hint.RangeName << ": " << Hint.Label;
+  friend llvm::raw_ostream <<(llvm::raw_ostream ,
+   const ExpectedHint ) {
+return Stream << Hint.Label << "@$" << Hint.RangeName;
   }
 };
 
-MATCHER_P2(HintMatcher, Expected, Code, "") {
-  return arg.label == Expected.Label &&
- arg.range == Code.range(Expected.RangeName) &&
- arg.position ==
- ((Expected.Side == Left) ? arg.range.start : arg.range.end);
+MATCHER_P2(HintMatcher, Expected, Code, llvm::to_string(Expected)) {
+  if (arg.label != Expected.Label) {
+*result_listener << "label is " << arg.label;
+return false;
+  }
+  if (arg.range != Code.range(Expected.RangeName)) {
+*result_listener << "range is " << arg.label << " but $"
+ << Expected.RangeName << " is "
+ << llvm::to_string(Code.range(Expected.RangeName));
+return false;
+  }
+  return true;
 }
 
 MATCHER_P(labelIs, Label, "") { return arg.label == Label; }
@@ -64,6 +72,7 @@
   Config C;
   C.InlayHints.Parameters = false;
   C.InlayHints.DeducedTypes = false;
+  C.InlayHints.Designators = false;
   return C;
 }
 
@@ -100,6 +109,15 @@
   assertHints(InlayHintKind::TypeHint, AnnotatedSource, Expected...);
 }
 
+template 
+void assertDesignatorHints(llvm::StringRef AnnotatedSource,
+   ExpectedHints... Expected) {
+  Config Cfg;
+  Cfg.InlayHints.Designators = true;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  assertHints(InlayHintKind::DesignatorHint, AnnotatedSource, Expected...);
+}
+
 TEST(ParameterHints, Smoke) {
   assertParameterHints(R"cpp(
 void foo(int param);
@@ -658,6 +676,71 @@
   ExpectedHint{": int", "var"});
 }
 
+TEST(DesignatorHints, Basic) {
+  assertDesignatorHints(R"cpp(
+struct S { int x, y, z; };
+S s {$x[[1]], $y[[2+2]]};
+
+int x[] = {$0[[0]], $1[[1]]};
+  )cpp",
+ExpectedHint{".x=", "x"}, ExpectedHint{".y=", "y"},
+ExpectedHint{"[0]=", "0"}, ExpectedHint{"[1]=", "1"});
+}
+
+TEST(DesignatorHints, Nested) {
+  assertDesignatorHints(R"cpp(
+struct Inner { int x, y; };
+struct Outer { Inner a, b; };
+Outer o{ $a[[{ $x[[1]], $y[[2]] }]], $bx[[3]] };
+  )cpp",
+ExpectedHint{".a=", "a"}, ExpectedHint{".x=", "x"},
+ExpectedHint{".y=", "y"}, ExpectedHint{".b.x=", "bx"});
+}
+
+TEST(DesignatorHints, AnonymousRecord) {
+  assertDesignatorHints(R"cpp(
+struct S {
+  union {
+struct {
+  struct {
+int y;
+  };
+} x;
+  };
+};
+S s{$xy[[42]]};
+  )cpp",
+ExpectedHint{".x.y=", "xy"});
+}
+
+TEST(DesignatorHints, Suppression) {
+  assertDesignatorHints(R"cpp(
+struct Point { int a, b, c, d, e, f, g, h; };
+Point p{/*a=*/1, .c=2, /* .d = */3, $e[[4]]};
+  )cpp",
+ExpectedHint{".e=", "e"});
+}
+
+TEST(DesignatorHints, StdArray) {
+  // Designators for std::array should be [0] rather than .__elements[0].
+  // While technically correct, the designator is useless and horrible to read.
+  assertDesignatorHints(R"cpp(
+

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

> Agree. I guess then we ignore the issue for this patch? As you say that's 
> affects other hints too, and a mitigation (hopefully) won't be specific to 
> the kind of hint.

Yup.


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

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

[Nathan Ridge via Phabricator via cfe-commits]

nridge accepted this revision.
nridge added a comment.
This revision is now accepted and ready to land.

Thanks, I'm happy to try the array designators and see how they work out in 
practice.

The issue of hints appearing and disappearing as you type a function call is 
one I've noticed, but I've gotten used to it after a while. Improvements in 
this area would be nice but probably don't need to block the addition of new 
hints.


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

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D116786#3260970 , @kadircet wrote:

> I feel like this is the way to go, but we'll need to put more thought into 
> the design

Agree. I guess then we ignore the issue for this patch? As you say that's 
affects other hints too, and a mitigation (hopefully) won't be specific to the 
kind of hint.


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

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

I was also thinking about a cache-based solution in which we can update results 
as we please, and as you noted the idea definitely generalizes to other 
requests as well, with possibly little nuances around the definition of "as we 
please", which makes things a lot more annoying as we can't have this as a 
single layer between all requests/responses but will require some feature 
specific customization.
I feel like this is the way to go, but we'll need to put more thought into the 
design especially considering the future improvements like pseudoparser and the 
way it's possibly going to change how these features are going to look like in 
presence of it. Maybe all of these features would work on top of a fresh 
pseudo-parse + last "good" ast :shrug:


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

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D116786#3257932 , @kadircet wrote:

>> Any ideas? :-(
>
> I've got a feeling that, this should also be annoying in the case of function 
> calls (especially when there's only one overload)? Maybe we should just 
> debounce the inlayhint requests (by saying contents modified to the clients)?

This seems like the right track, but I'm not sure rejecting requests 
specifically is the right answer. Two reasons:

1. This requires careful impl on the client side. LSP has been amended to 
greatly discourage contentmodified. We still send it heuristically, but there's 
a capability to opt out of this and vscode sets it.

(As long as this is a custom protocol we can say what we want, but it won't be 
forever).

2. Choosing which requests to respond to is hard, stateful work.

This isn't the drop-for-efficiency situation where the queue is full, this 
behavior is most annoying when the server is able to keep up with keystrokes. 
And the errors that cause this may not be trivially brief "i'm in the middle of 
typing a word" anyway.
Really you want to work out if this is a "good" request based on the AST. And 
in fact the analysis you'll do looks a lot like actually generating the hints, 
and probably comparing with the last ones. Then we throw them all away?!

---

I'd suggest something slightly different: we compute inlay hints, heuristically 
merge with some previous state, and return the merged results.
Heuristics are something like:

- generally prefer freshly computed hints
- be willing to add old ones where newer ones are missing
- be willing to use older ones where code has changed near the hints (but not 
when they overlap!)

(There's not much here that's specific to inlay hints vs other decorations tied 
to ranges - certainly we could/should do this for semantictokens too, and 
documentSymbols, ...)

---

A variation on this idea is to compute these decorations frequently but 
"whenever we like" and always serve them from this cache, just patching them 
for changes in the source code.
This gives us more control of when the analyses run, maybe there's a lot of 
extra efficiency here. But I think the cache still needs stateful updates to 
solve this bug, unless we have *great* heuristics for "whenever we like".


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

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

> Any ideas? :-(

I've got a feeling that, this should also be annoying in the case of function 
calls (especially when there's only one overload)? Maybe we should just 
debounce the inlayhint requests (by saying contents modified to the clients)?


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

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In 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 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

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

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 401410.
sammccall marked 8 inline comments as done.
sammccall added a comment.

Address comments, rebase.
This category is *off* by default.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116786

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -13,21 +13,22 @@
 #include "TestWorkspace.h"
 #include "XRefs.h"
 #include "support/Context.h"
+#include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 
-std::ostream <<(std::ostream , const InlayHint ) {
-  return Stream << Hint.label;
+llvm::raw_ostream <<(llvm::raw_ostream ,
+  const InlayHint ) {
+  return Stream << Hint.label << "@" << Hint.range;
 }
 
 namespace {
 
 using ::testing::ElementsAre;
 using ::testing::IsEmpty;
-using ::testing::UnorderedElementsAre;
 
 std::vector hintsOfKind(ParsedAST , InlayHintKind Kind) {
   std::vector Result;
@@ -45,17 +46,24 @@
   std::string RangeName;
   HintSide Side = Left;
 
-  friend std::ostream <<(std::ostream ,
-  const ExpectedHint ) {
-return Stream << Hint.RangeName << ": " << Hint.Label;
+  friend llvm::raw_ostream <<(llvm::raw_ostream ,
+   const ExpectedHint ) {
+return Stream << Hint.Label << "@$" << Hint.RangeName;
   }
 };
 
-MATCHER_P2(HintMatcher, Expected, Code, "") {
-  return arg.label == Expected.Label &&
- arg.range == Code.range(Expected.RangeName) &&
- arg.position ==
- ((Expected.Side == Left) ? arg.range.start : arg.range.end);
+MATCHER_P2(HintMatcher, Expected, Code, llvm::to_string(Expected)) {
+  if (arg.label != Expected.Label) {
+*result_listener << "label is " << arg.label;
+return false;
+  }
+  if (arg.range != Code.range(Expected.RangeName)) {
+*result_listener << "range is " << arg.label << " but $"
+ << Expected.RangeName << " is "
+ << llvm::to_string(Code.range(Expected.RangeName));
+return false;
+  }
+  return true;
 }
 
 MATCHER_P(labelIs, Label, "") { return arg.label == Label; }
@@ -64,6 +72,7 @@
   Config C;
   C.InlayHints.Parameters = false;
   C.InlayHints.DeducedTypes = false;
+  C.InlayHints.Designators = false;
   return C;
 }
 
@@ -100,6 +109,15 @@
   assertHints(InlayHintKind::TypeHint, AnnotatedSource, Expected...);
 }
 
+template 
+void assertDesignatorHints(llvm::StringRef AnnotatedSource,
+   ExpectedHints... Expected) {
+  Config Cfg;
+  Cfg.InlayHints.Designators = true;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  assertHints(InlayHintKind::DesignatorHint, AnnotatedSource, Expected...);
+}
+
 TEST(ParameterHints, Smoke) {
   assertParameterHints(R"cpp(
 void foo(int param);
@@ -658,6 +676,71 @@
   ExpectedHint{": int", "var"});
 }
 
+TEST(DesignatorHints, Basic) {
+  assertDesignatorHints(R"cpp(
+struct S { int x, y, z; };
+S s {$x[[1]], $y[[2+2]]};
+
+int x[] = {$0[[0]], $1[[1]]};
+  )cpp",
+ExpectedHint{".x=", "x"}, ExpectedHint{".y=", "y"},
+ExpectedHint{"[0]=", "0"}, ExpectedHint{"[1]=", "1"});
+}
+
+TEST(DesignatorHints, Nested) {
+  assertDesignatorHints(R"cpp(
+struct Inner { int x, y; };
+struct Outer { Inner a, b; };
+Outer o{ $a[[{ $x[[1]], $y[[2]] }]], $bx[[3]] };
+  )cpp",
+ExpectedHint{".a=", "a"}, ExpectedHint{".x=", "x"},
+ExpectedHint{".y=", "y"}, ExpectedHint{".b.x=", "bx"});
+}
+
+TEST(DesignatorHints, AnonymousRecord) {
+  assertDesignatorHints(R"cpp(
+struct S {
+  union {
+struct {
+  struct {
+int y;
+  };
+} x;
+  };
+};
+S s{$xy[[42]]};
+  )cpp",
+ExpectedHint{".x.y=", "xy"});
+}
+
+TEST(DesignatorHints, Suppression) {
+  assertDesignatorHints(R"cpp(
+struct Point { int a, b, c, d, e, f, g, h; };
+Point p{/*a=*/1, .c=2, /* .d = */3, $e[[4]]};
+  )cpp",
+ExpectedHint{".e=", "e"});
+}
+
+TEST(DesignatorHints, StdArray) {
+  // Designators for std::array should be [0] rather than .__elements[0].
+  // While technically correct, the designator is useless and horrible to read.
+  assertDesignatorHints(R"cpp(
+template  

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

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.


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

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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Thanks, this is a pretty nice addition!

My only piece of high-level feedback is probing if the array element 
designators (`[0]=`) are useful enough to be worth the space/noise. The rest is 
minor implementation comments.




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) {

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)



Comment at: clang-tools-extra/clangd/InlayHints.cpp:81
+   // std::array x = {1,2,3}. Designators not strictly valid!
+   (OneField && isReservedName(FieldName
+return true;

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.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:99
+  unsigned Index = 0;
+  CXXRecordDecl::base_class_const_iterator BasesI;
+  CXXRecordDecl::base_class_const_iterator BasesE;

suggest `Iter` and `End`, `I` and `E` are a bit cryptic



Comment at: clang-tools-extra/clangd/InlayHints.cpp:117
+// It should generate designators '.a:' and '.b.x:'.
+// '.a:' is produced directly without recursing into the written sublist.
+// Recursion with Prefix='.b' and Sem = {3, ImplicitValue} produces '.b.x:'.

Can we add that the written sublist will get its own VisitInitListExpr call by 
RAV, while the implicit sublist will not? (If I've understood that correctly.)



Comment at: clang-tools-extra/clangd/InlayHints.cpp:128
+  // The elements of the semantic form all correspond to direct subobjects of
+  // the type Type. `Fields` iterates over these subobject names.
+  AggregateDesignatorNames Fields(Sem->getType());

"of the aggregate type"? ("the type Type" is a bit confusing, for a moment I 
thought you were talking about `clang::Type`)



Comment at: clang-tools-extra/clangd/InlayHints.cpp:145
+
+if (!Fields.append(Prefix, BraceElidedSubobject != nullptr))
+  continue; // no designator available for this subobject

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...



Comment at: clang-tools-extra/clangd/InlayHints.cpp:151
+  // Descend into the semantic list describing the subobject.
+  collectDesignators(BraceElidedSubobject, Out, NestedBraces, Prefix);
+  continue;

It took me a while to think through why it's **not** necessary to add more 
things into `NestedBraces` for the recursive call: since we only recurse in the 
case of elided braces, if a brace-elided subobject has an initializer with an 
explicit brace then **syntactically** that's a direct child of the containing 
explicit-brace initializer (even if **semantically** it's several levels deep), 
and thus its brace is already represented in `NestedBraces`. Might be worth 
pointing that out in a comment.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:165
+
+  // gatherDesignators needs to know which InitListExprs in the semantic tree
+  // were actually written, but InitListExpr::isExplicit() lies.

collectDesignators



Comment at: clang-tools-extra/clangd/InlayHints.cpp:176
+  llvm::DenseMap Designators;
+  std::string Scratch;
+  collectDesignators(Syn->isSemanticForm() ? Syn : Syn->getSemanticForm(),

"EmptyPrefix"? ("Scratch" makes it sound like "scratch space for it to write 
temporary things into")



Comment at: clang-tools-extra/clangd/InlayHints.cpp:283
 
+  bool VisitInitListExpr(InitListExpr *Syn) {
+if (Syn->isIdiomaticZeroInitializer(AST.getLangOpts()))

I take it the parameter name `Syn` is supposed to suggest that the 
`InitListExpr` is the syntactic form (of the two forms described 
[here](https://searchfox.org/llvm/rev/be9eafc71004393363d155dd16ea1af9c663aafe/clang/include/clang/AST/Expr.h#4757)),
 and that we know this because `InlayHintVisitor` does not override 
`shouldVisitImplicitCode()`, which makes RAV [only 
traverse](https://searchfox.org/llvm/rev/be9eafc71004393363d155dd16ea1af9c663aafe/clang/include/clang/AST/RecursiveASTVisitor.h#2412)
 the syntactic form.

I think it would aid understandability if we mentioned these things in a 
comment.



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.

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

(I know you're on vacation, this isn't urgent, just wanted to send it so I 
didn't lose it to git obscurity!)




Comment at: clang-tools-extra/clangd/InlayHints.cpp:400
   return false;
-// Allow whitespace and "=" at end of comment.
-SourcePrefix = SourcePrefix.rtrim().rtrim('=').rtrim();
+// Ignore some punctuation and whitespace around comment.
+// In particular this allows designators to match nicely.

This makes the existing comment suppression (for param names) a bit sloppier, 
but I have a hard time imagining a case where punctuation makes a difference.


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

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

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: nridge.
Herald added subscribers: usaxena95, kadircet, arphaman.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

These make the init lists appear as if designated initialization was used.

Example:

  ExpectedHint{"param: ", "arg"}

becomes

  ExpectedHint{.Label="param: ", .RangeName="arg"}


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116786

Files:
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -11,18 +11,21 @@
 #include "TestTU.h"
 #include "TestWorkspace.h"
 #include "XRefs.h"
+#include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 
-std::ostream <<(std::ostream , const InlayHint ) {
-  return Stream << Hint.label;
+llvm::raw_ostream <<(llvm::raw_ostream ,
+  const InlayHint ) {
+  return Stream << Hint.label << "@" << Hint.range;
 }
 
 namespace {
 
+using ::testing::ElementsAre;
 using ::testing::UnorderedElementsAre;
 
 std::vector hintsOfKind(ParsedAST , InlayHintKind Kind) {
@@ -38,15 +41,24 @@
   std::string Label;
   std::string RangeName;
 
-  friend std::ostream <<(std::ostream ,
-  const ExpectedHint ) {
-return Stream << Hint.RangeName << ": " << Hint.Label;
+  friend llvm::raw_ostream <<(llvm::raw_ostream ,
+   const ExpectedHint ) {
+return Stream << Hint.Label << "@$" << Hint.RangeName;
   }
 };
 
-MATCHER_P2(HintMatcher, Expected, Code, "") {
-  return arg.label == Expected.Label &&
- arg.range == Code.range(Expected.RangeName);
+MATCHER_P2(HintMatcher, Expected, Code, llvm::to_string(Expected)) {
+  if (arg.label != Expected.Label) {
+*result_listener << "label is " << arg.label;
+return false;
+  }
+  if (arg.range != Code.range(Expected.RangeName)) {
+*result_listener << "range is " << arg.label << " but $"
+ << Expected.RangeName << " is "
+ << llvm::to_string(Code.range(Expected.RangeName));
+return false;
+  }
+  return true;
 }
 
 template 
@@ -58,7 +70,7 @@
   auto AST = TU.build();
 
   EXPECT_THAT(hintsOfKind(AST, Kind),
-  UnorderedElementsAre(HintMatcher(Expected, Source)...));
+  ElementsAre(HintMatcher(Expected, Source)...));
 }
 
 template 
@@ -73,6 +85,12 @@
   assertHints(InlayHintKind::TypeHint, AnnotatedSource, Expected...);
 }
 
+template 
+void assertDesignatorHints(llvm::StringRef AnnotatedSource,
+   ExpectedHints... Expected) {
+  assertHints(InlayHintKind::DesignatorHint, AnnotatedSource, Expected...);
+}
+
 TEST(ParameterHints, Smoke) {
   assertParameterHints(R"cpp(
 void foo(int param);
@@ -631,6 +649,71 @@
   ExpectedHint{": int", "var"});
 }
 
+TEST(DesignatorHints, Basic) {
+  assertDesignatorHints(R"cpp(
+struct S { int x, y, z; };
+S s {$x[[1]], $y[[2+2]]};
+
+int x[] = {$0[[0]], $1[[1]]};
+  )cpp",
+ExpectedHint{".x=", "x"}, ExpectedHint{".y=", "y"},
+ExpectedHint{"[0]=", "0"}, ExpectedHint{"[1]=", "1"});
+}
+
+TEST(DesignatorHints, Nested) {
+  assertDesignatorHints(R"cpp(
+struct Inner { int x, y; };
+struct Outer { Inner a, b; };
+Outer o{ $a[[{ $x[[1]], $y[[2]] }]], $bx[[3]] };
+  )cpp",
+ExpectedHint{".a=", "a"}, ExpectedHint{".x=", "x"},
+ExpectedHint{".y=", "y"}, ExpectedHint{".b.x=", "bx"});
+}
+
+TEST(DesignatorHints, AnonymousRecord) {
+  assertDesignatorHints(R"cpp(
+struct S {
+  union {
+struct {
+  struct {
+int y;
+  };
+} x;
+  };
+};
+S s{$xy[[42]]};
+  )cpp",
+ExpectedHint{".x.y=", "xy"});
+}
+
+TEST(DesignatorHints, Suppression) {
+  assertDesignatorHints(R"cpp(
+struct Point { int a, b, c, d, e, f, g, h; };
+Point p{/*a=*/1, .c=2, /* .d = */3, $e[[4]]};
+  )cpp",
+ExpectedHint{".e=", "e"});
+}
+
+TEST(DesignatorHints, StdArray) {
+  // Designators for std::array should be [0] rather than .__elements[0].
+  // While technically correct, the designator is useless and horrible to read.
+  assertDesignatorHints(R"cpp(
+template  struct Array { T __elements[N]; };
+Array x = {$0[[0]], $1[[1]]};
+  )cpp",
+ExpectedHint{"[0]=", "0"}, ExpectedHint{"[1]=", "1"});
+}
+
+TEST(DesignatorHints, OnlyAggregateInit) {
+