[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<int,3> 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<SourceLocation, std::string> 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.
----------------
Why do we need to trim `ParamName`?


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:660
+                        ExpectedHint{".x=", "x"}, ExpectedHint{".y=", "y"},
+                        ExpectedHint{"[0]=", "0"}, ExpectedHint{"[1]=", "1"});
+}
----------------
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...


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:713
+    struct Constructible { Constructible(int x); };
+    Constructible x{42};
+  )cpp" /*no hints expected*/);
----------------
Maybe add a clarifying comment saying "this argument gets a hint, but not of type Designator", as this confused me for a while.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116786