This is an archive of the discontinued LLVM Phabricator instance.

[CodeCompletion] Signature help for aggregate initialization.
ClosedPublic

Authored by sammccall on Dec 27 2021, 7:37 PM.

Details

Summary

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:

Diff Detail

Event Timeline

sammccall created this revision.Dec 27 2021, 7:37 PM
sammccall requested review of this revision.Dec 27 2021, 7:37 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 27 2021, 7:37 PM
nridge added a subscriber: nridge.Jan 1 2022, 4:55 PM
kadircet added inline comments.Jan 3 2022, 4:41 AM
clang-tools-extra/clangd/CodeComplete.cpp
1190

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)

clang/include/clang/Sema/CodeCompleteConsumer.h
1075

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)

clang/lib/Sema/CodeCompleteConsumer.cpp
523

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
clang/lib/Sema/SemaCodeComplete.cpp
6107

also static

sammccall marked 2 inline comments as done.Jan 3 2022, 8:45 AM
sammccall added inline comments.
clang-tools-extra/clangd/CodeComplete.cpp
1190

I don't think I've changed behavior here, if there's no name then the IdentifierInfo is nullptr

clang/include/clang/Sema/CodeCompleteConsumer.h
1075

I can't remember why I added it now!
I've added the others, tests still pass.

clang/lib/Sema/CodeCompleteConsumer.cpp
523

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.

sammccall updated this revision to Diff 397075.Jan 3 2022, 8:45 AM
sammccall marked an inline comment as done.

address comments

usaxena95 added inline comments.Jan 3 2022, 11:51 PM
clang/lib/Sema/SemaCodeComplete.cpp
3699

nit: const ref.

6094

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

6132

nit: move closer to usage below.

kadircet added inline comments.Jan 4 2022, 12:47 AM
clang/include/clang/Sema/CodeCompleteConsumer.h
1095

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

clang/lib/Sema/CodeCompleteConsumer.cpp
523

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.

569

this doesn't cover the function(proto)type case

clang/lib/Sema/SemaCodeComplete.cpp
3903–3904

isn't this and the following case actually the same? can't we just use the return type inside functiontype ?

5958

nit: early exit

6105

assert that Aggregate is of right kind?

sammccall marked 11 inline comments as done.Jan 4 2022, 3:22 AM
sammccall added inline comments.
clang/lib/Sema/CodeCompleteConsumer.cpp
569

That's right. Function(Proto)Type doesn't contain decls for the parameters. Added a comment.

clang/lib/Sema/SemaCodeComplete.cpp
6094

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
sammccall updated this revision to Diff 397251.Jan 4 2022, 3:24 AM
sammccall marked 2 inline comments as done.

Address comments

kadircet accepted this revision.Jan 4 2022, 5:47 AM

thanks, lgtm!

This revision is now accepted and ready to land.Jan 4 2022, 5:47 AM
usaxena95 accepted this revision.Jan 4 2022, 5:55 AM
This revision was landed with ongoing or failed builds.Jan 4 2022, 7:00 AM
This revision was automatically updated to reflect the committed changes.