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