[PATCH] D116326: [CodeCompletion] Signature help for aggregate initialization.

[Sam McCall via Phabricator via cfe-commits]

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<CXXRecordDecl>(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