ilya-biryukov added a comment.
Mostly NITs
================
Comment at: lib/Sema/SemaCodeComplete.cpp:5136
+ auto AddDefaultCtorInit = [&](const char *TypedName,
+ const char *TypeName,
+ const NamedDecl* ND) {
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > Maybe use StringRef?
> Don't think this looks any better, since we need to use `.data()` at chunk
> additions than.
>
> Even if we try to push allocations all the way down until we add those
> chunks, we still get bad looking code since types are returned as
> `std::string` and we need to make a copy in the stack to be able to pass it
> as `StringRef`.
`char*` and `StringRef` have equivalent lifetime properties, so the set of
cases you need to do extra copies is equivalent. Am I missing something?
But yeah, since AddChunk methods accept a `char*`, `StringRef` is not suitable
because it's not guaranteed to be null-terminated. LG
================
Comment at: lib/Sema/SemaCodeComplete.cpp:813
+DeclContext::lookup_result getConstructorResults(ASTContext &Context,
+ const CXXRecordDecl *Record) {
----------------
Maybe make the name shorter?
E.g. `getConstructors` or `lookupConstructors`?
================
Comment at: lib/Sema/SemaCodeComplete.cpp:845
+ auto Ctors = getConstructorResults(SemaRef.Context, Record);
+ for(auto Ctor : Ctors) {
+ R.Declaration = Ctor;
----------------
Maybe inline `Ctors`?
I.e.
```
for (auto Ctor : getConstructorResults(SemaRef.Context, Record))
```
================
Comment at: lib/Sema/SemaCodeComplete.cpp:5086
+ auto GenerateCCS = [&](const NamedDecl *ND, const char *Name) {
+ Builder.AddTypedTextChunk(Name);
+ Builder.AddChunk(CodeCompletionString::CK_LeftParen);
----------------
`Builder` is not used outside `GenerateCCS` and `AddDefaultCtorInit`, maybe
move its declaration into those lamdbas and remove from the enclosing function?
Would mean less mutable state to care about and the function seems big enough
for that to matter.
================
Comment at: lib/Sema/SemaCodeComplete.cpp:5088
+ Builder.AddChunk(CodeCompletionString::CK_LeftParen);
+ if (const FunctionDecl *Function = dyn_cast<FunctionDecl>(ND))
+ AddFunctionParameterChunks(PP, Policy, Function, Builder);
----------------
NIT: use auto
================
Comment at: lib/Sema/SemaCodeComplete.cpp:5090
+ AddFunctionParameterChunks(PP, Policy, Function, Builder);
+ else if (const FunctionTemplateDecl *FunTemplDecl =
+ dyn_cast<FunctionTemplateDecl>(ND))
----------------
NIT: use auto
================
Comment at: test/CodeCompletion/ctor-initializer.cpp:44
+// RUN: %clang_cc1 -fsyntax-only -std=c++14 -code-completion-at=%s:38:39 %s -o
- | FileCheck -check-prefix=CHECK-CC5 %s
+// CHECK-CC5-NOT: COMPLETION: Pattern : Base1()
+// CHECK-CC5-NOT: COMPLETION: Pattern : Base1(<#int#>)
----------------
Maybe use a single check `CHECK-CC5-NOT: COMPLETION: Pattern : Base`? (i.e. not
mentioning the arguments)
================
Comment at: test/CodeCompletion/ctor-initializer.cpp:69
struct Base2 {
- Base2(int);
+ Base2(int, float x = 3);
};
----------------
Why do we want to change this test?
================
Comment at: test/Index/complete-ctor-inits.cpp:33
// RUN: c-index-test -code-completion-at=%s:18:10 %s | FileCheck
-check-prefix=CHECK-CC1 %s
-// CHECK-CC1: MemberRef:{TypedText a}{LeftParen (}{Placeholder
args}{RightParen )} (35)
-// CHECK-CC1: MemberRef:{TypedText b}{LeftParen (}{Placeholder
args}{RightParen )} (35)
----------------
Reading through the code, it seems `MemberRef` was the correct CursorKind here
(the docs mention that MemberRef is a reference in non-expression context and
ctor-initializer seems to be one of those).
Maybe keep it a `MemberRef`, since it looks simple enough.
Having CXXConstrutor instead of NotImpleneted is clearly a win, though :-)
Repository:
rC Clang
https://reviews.llvm.org/D53654
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
