[PATCH] D53654: [clang] Improve ctor initializer completions.

[Ilya Biryukov via Phabricator via cfe-commits]

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