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