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