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

[Kadir Cetinkaya via Phabricator via cfe-commits]

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<CXXRecordDecl>(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<unsigned>
+getNextAggregateIndexAfterDesignatedInit(const ResultCandidate &Aggregate,
+                                         ArrayRef<Expr *> 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