[PATCH] D116326: [CodeCompletion] Signature help for aggregate initialization.
kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks, lgtm! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116326/new/ https://reviews.llvm.org/D116326 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D116326: [CodeCompletion] Signature help for aggregate initialization.
sammccall updated this revision to Diff 397251.
sammccall marked 2 inline comments as done.
sammccall added a comment.
Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116326/new/
https://reviews.llvm.org/D116326
Files:
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
clang/include/clang/Sema/CodeCompleteConsumer.h
clang/lib/Sema/CodeCompleteConsumer.cpp
clang/lib/Sema/SemaCodeComplete.cpp
clang/test/CodeCompletion/ctor-signature.cpp
Index: clang/test/CodeCompletion/ctor-signature.cpp
===
--- clang/test/CodeCompletion/ctor-signature.cpp
+++ clang/test/CodeCompletion/ctor-signature.cpp
@@ -42,13 +42,29 @@
// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:41:22 %s | FileCheck -check-prefix=CHECK-BRACED %s
struct Aggregate {
- // FIXME: no support for aggregates yet.
- // CHECK-AGGREGATE-NOT: OVERLOAD: Aggregate{<#const Aggregate
[PATCH] D116326: [CodeCompletion] Signature help for aggregate initialization.
sammccall marked 11 inline comments as done.
sammccall added inline comments.
Comment at: clang/lib/Sema/CodeCompleteConsumer.cpp:568
+
+ if (const auto *FD = getFunction()) {
+if (N < FD->param_size())
kadircet wrote:
> this doesn't cover the function(proto)type case
That's right. Function(Proto)Type doesn't contain decls for the parameters.
Added a comment.
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:6018
+// after `S{.b=1,` we want to suggest c to continue
+// after `S{.b=1, 2,` we continue with d (this is legal C and ext in C++)
+//
usaxena95 wrote:
> Can you also add detail for out-of-order designated initialisation.
> `S{.c=1, .b=2,` (valid C, invalid C++, ext in C++)
> IIUC the current version would continue with `c` here. (reasoning: promoting
> in-order designated initialisation ?)
Added a comment
The reasoning is just that's the correct behavior :-)
```
struct S {int a,b; };
int m = (struct S){.b=1, .a=2, 3}.b;
// now m.a is 3
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116326/new/
https://reviews.llvm.org/D116326
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D116326: [CodeCompletion] Signature help for aggregate initialization.
kadircet added inline comments.
Comment at: clang/include/clang/Sema/CodeCompleteConsumer.h:1085
+const RecordDecl *getAggregate() const {
+ return getKind() == CK_Aggregate ? AggregateType : nullptr;
+}
either assert the kind and return `AggregateType` or change the callers below
to not explicitly check for kind (and directly check if returned RecordDecl is
not-null).
Comment at: clang/lib/Sema/CodeCompleteConsumer.cpp:568
+
+ if (const auto *FD = getFunction()) {
+if (N < FD->param_size())
this doesn't cover the function(proto)type case
Comment at: clang/lib/Sema/CodeCompleteConsumer.cpp:522
+if (const auto *CRD = dyn_cast(AggregateType))
+ Count += CRD->getNumBases();
+return Count;
sammccall wrote:
> kadircet wrote:
> > i think this is only valid for c++17 onwards `If the number of initializer
> > clauses exceeds the number of members and bases (since C++17) to
> > initialize, the program is ill-formed.` (same for param type/decl)
> >
> > it gets even more complicated, since one doesn't have to nest
> > initialization of base class fields, e.g this is valid:
> > ```
> > struct X
> > {
> > int a;
> > int b;
> > };
> >
> > struct Y : X
> > {
> > int c;
> > };
> >
> > Y y1{ {1,2}, 3};
> > Y y2{ 1, 2, 3 }; // both of these set a=1, b=2, c=3
> > ```
> > i think this is only valid for c++17 onwards
>
> We only get here for aggregate types, and pre-C++17 those are guaranteed to
> have no bases.
>
> > one doesn't have to nest initialization of base class fields
>
> Great catch, somehow I missed this in the spec. This makes things very
> complicated. Why on earth is this allowed?! (After some digging,
> compatibility with C99)
>
> I can't see a reasonable way to support this. If it were just bases I'd drop
> support for them, but it applies to fields too.
> The implementation difficulty is having to do full conversion-sequence checks
> for each element to tell whether it is a standalone element or the first in
> an implied nested list.
>
> This elision is slightly discouraged (-Wmissing-braces in in -Wall) so I
> think it's OK to give results that are going to be a bit off if brace-elision
> is used.
> If there are a lot of complaints we could try to detect this in future.
>
>
> This elision is slightly discouraged (-Wmissing-braces in in -Wall) so I
> think it's OK to give results that are going to be a bit off if brace-elision
> is used.
If there are a lot of complaints we could try to detect this in future.
As discussed offline I second this.
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3826
Result.AddTextChunk(Result.getAllocator().CopyString(OS.str()));
- } else {
+ } else if (Proto) {
Result.AddResultTypeChunk(Result.getAllocator().CopyString(
isn't this and the following case actually the same? can't we just use the
return type inside functiontype ?
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5884
+QualType CandidateParamType = Candidate.getParamType(N);
+if (!CandidateParamType.isNull()) {
+ if (ParamType.isNull())
nit: early exit
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:6029
+static llvm::Optional
+getNextAggregateIndexAfterDesignatedInit(const ResultCandidate ,
+ ArrayRef Args) {
assert that `Aggregate` is of right kind?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116326/new/
https://reviews.llvm.org/D116326
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D116326: [CodeCompletion] Signature help for aggregate initialization.
usaxena95 added inline comments.
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3699
+ unsigned ChunkIndex = 0;
+ auto AddChunk = [&](std::string Placeholder) {
+if (ChunkIndex > 0)
nit: const ref.
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:6018
+// after `S{.b=1,` we want to suggest c to continue
+// after `S{.b=1, 2,` we continue with d (this is legal C and ext in C++)
+//
Can you also add detail for out-of-order designated initialisation.
`S{.c=1, .b=2,` (valid C, invalid C++, ext in C++)
IIUC the current version would continue with `c` here. (reasoning: promoting
in-order designated initialisation ?)
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:6056
+ // (Probing getParamDecl() directly would be quadratic in number of fields).
+ unsigned AggregateSize = Aggregate.getNumParams();
+ unsigned DesignatedIndex = 0;
nit: move closer to usage below.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116326/new/
https://reviews.llvm.org/D116326
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D116326: [CodeCompletion] Signature help for aggregate initialization.
sammccall updated this revision to Diff 397075.
sammccall marked an inline comment as done.
sammccall added a comment.
address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116326/new/
https://reviews.llvm.org/D116326
Files:
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
clang/include/clang/Sema/CodeCompleteConsumer.h
clang/lib/Sema/CodeCompleteConsumer.cpp
clang/lib/Sema/SemaCodeComplete.cpp
clang/test/CodeCompletion/ctor-signature.cpp
Index: clang/test/CodeCompletion/ctor-signature.cpp
===
--- clang/test/CodeCompletion/ctor-signature.cpp
+++ clang/test/CodeCompletion/ctor-signature.cpp
@@ -42,13 +42,29 @@
// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:41:22 %s | FileCheck -check-prefix=CHECK-BRACED %s
struct Aggregate {
- // FIXME: no support for aggregates yet.
- // CHECK-AGGREGATE-NOT: OVERLOAD: Aggregate{<#const Aggregate
[PATCH] D116326: [CodeCompletion] Signature help for aggregate initialization.
sammccall marked 2 inline comments as done.
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1184
- auto Name = Ident->getName();
- if (!Name.empty())
-ParamNames.insert(Name.str());
kadircet wrote:
> we were never recording unnamed params before, now they'll be surfaced during
> comment code completions. can you either keep dropping them here or on the
> code completion generation side (~line 1997)
I don't think I've changed behavior here, if there's no name then the
IdentifierInfo is nullptr
Comment at: clang/include/clang/Sema/CodeCompleteConsumer.h:1057
+: Kind(CK_Aggregate), AggregateType(Aggregate) {
+ assert(Aggregate != nullptr);
+}
kadircet wrote:
> we don't have assertions in other cases, why here? (i think we should have it
> in other cases too, just wondering if there are any valid states where we
> construct an overloadcandidate from nullptr)
I can't remember why I added it now!
I've added the others, tests still pass.
Comment at: clang/lib/Sema/CodeCompleteConsumer.cpp:522
+if (const auto *CRD = dyn_cast(AggregateType))
+ Count += CRD->getNumBases();
+return Count;
kadircet wrote:
> i think this is only valid for c++17 onwards `If the number of initializer
> clauses exceeds the number of members and bases (since C++17) to initialize,
> the program is ill-formed.` (same for param type/decl)
>
> it gets even more complicated, since one doesn't have to nest initialization
> of base class fields, e.g this is valid:
> ```
> struct X
> {
> int a;
> int b;
> };
>
> struct Y : X
> {
> int c;
> };
>
> Y y1{ {1,2}, 3};
> Y y2{ 1, 2, 3 }; // both of these set a=1, b=2, c=3
> ```
> i think this is only valid for c++17 onwards
We only get here for aggregate types, and pre-C++17 those are guaranteed to
have no bases.
> one doesn't have to nest initialization of base class fields
Great catch, somehow I missed this in the spec. This makes things very
complicated. Why on earth is this allowed?! (After some digging, compatibility
with C99)
I can't see a reasonable way to support this. If it were just bases I'd drop
support for them, but it applies to fields too.
The implementation difficulty is having to do full conversion-sequence checks
for each element to tell whether it is a standalone element or the first in an
implied nested list.
This elision is slightly discouraged (-Wmissing-braces in in -Wall) so I think
it's OK to give results that are going to be a bit off if brace-elision is used.
If there are a lot of complaints we could try to detect this in future.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116326/new/
https://reviews.llvm.org/D116326
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D116326: [CodeCompletion] Signature help for aggregate initialization.
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1184
- auto Name = Ident->getName();
- if (!Name.empty())
-ParamNames.insert(Name.str());
we were never recording unnamed params before, now they'll be surfaced during
comment code completions. can you either keep dropping them here or on the code
completion generation side (~line 1997)
Comment at: clang/include/clang/Sema/CodeCompleteConsumer.h:1057
+: Kind(CK_Aggregate), AggregateType(Aggregate) {
+ assert(Aggregate != nullptr);
+}
we don't have assertions in other cases, why here? (i think we should have it
in other cases too, just wondering if there are any valid states where we
construct an overloadcandidate from nullptr)
Comment at: clang/lib/Sema/CodeCompleteConsumer.cpp:522
+if (const auto *CRD = dyn_cast(AggregateType))
+ Count += CRD->getNumBases();
+return Count;
i think this is only valid for c++17 onwards `If the number of initializer
clauses exceeds the number of members and bases (since C++17) to initialize,
the program is ill-formed.` (same for param type/decl)
it gets even more complicated, since one doesn't have to nest initialization of
base class fields, e.g this is valid:
```
struct X
{
int a;
int b;
};
struct Y : X
{
int c;
};
Y y1{ {1,2}, 3};
Y y2{ 1, 2, 3 }; // both of these set a=1, b=2, c=3
```
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:6031
+ ArrayRef Args) {
+ constexpr unsigned Invalid = std::numeric_limits::max();
+
also `static`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116326/new/
https://reviews.llvm.org/D116326
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D116326: [CodeCompletion] Signature help for aggregate initialization.
sammccall created this revision.
sammccall added reviewers: kadircet, usaxena95.
Herald added a subscriber: arphaman.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, ilya-biryukov.
Herald added projects: clang, clang-tools-extra.
The "parameter list" is the list of fields which should be initialized.
We introduce a new OverloadCandidate kind for this.
It starts to become harder for CC consumers to handle all the cases for
params, so I added some extra APIs on OverloadCandidate to abstract them.
Includes some basic support for designated initializers.
The same aggregate signature is shown, the current arg jumps after the
one you just initialized. This follows C99 semantics for mixed
designated/positional initializers (which clang supports in C++ as an extension)
and is also a useful prompt for C++ as C++ designated initializers must be
in order.
Related bugs:
- https://github.com/clangd/clangd/issues/965
- https://github.com/clangd/clangd/issues/306
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D116326
Files:
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
clang/include/clang/Sema/CodeCompleteConsumer.h
clang/lib/Sema/CodeCompleteConsumer.cpp
clang/lib/Sema/SemaCodeComplete.cpp
clang/test/CodeCompletion/ctor-signature.cpp
Index: clang/test/CodeCompletion/ctor-signature.cpp
===
--- clang/test/CodeCompletion/ctor-signature.cpp
+++ clang/test/CodeCompletion/ctor-signature.cpp
@@ -42,13 +42,29 @@
// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:41:22 %s | FileCheck -check-prefix=CHECK-BRACED %s
struct Aggregate {
- // FIXME: no support for aggregates yet.
- // CHECK-AGGREGATE-NOT: OVERLOAD: Aggregate{<#const Aggregate
