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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Great idea, this will definitely improve the UX of completion!

NIT: a typo in the change description: 'bas' should be 'base'.
Pedantic NIT (sorry!): in the change description, we should probably use 'initializers' instead of 'initializations'



================
Comment at: lib/Sema/SemaCodeComplete.cpp:815
+std::vector<CodeCompletionResult>
+ResultBuilder::GetConstructorResults(const CXXRecordDecl *Record, Result R) {
+  ASTContext &Context = SemaRef.Context;
----------------
Maybe add the ctor results directly to the result list without creating an intermediate vector?
See other comments about passing the field name to this function instead of replacing it in the resulting CCS afterwards.


================
Comment at: lib/Sema/SemaCodeComplete.cpp:5093
+
+  auto CreatePrimitiveConstructor = [&](StringRef Name) {
+    Builder.AddTypedTextChunk(Builder.getAllocator().CopyString(Name));
----------------
NIT: maybe pass the Builder explicitly? Even a local function that changes the state and without explicit data-flow (i.e. without accepting the thing it changes as a parameter) is a bit confusing?


================
Comment at: lib/Sema/SemaCodeComplete.cpp:5101
+    const auto *ND = Base.getType()->getAsCXXRecordDecl();
+    if (isa<ClassTemplateSpecializationDecl>(ND) ||
+        isa<ClassTemplatePartialSpecializationDecl>(ND)) {
----------------
Why special-case the template specializations?
Are we trying to provide results for dependent types here?


================
Comment at: lib/Sema/SemaCodeComplete.cpp:5103
+        isa<ClassTemplatePartialSpecializationDecl>(ND)) {
+      CreatePrimitiveConstructor(Base.getType().getAsString(Policy));
+      Results.AddResult(CodeCompletionResult(
----------------
It seems this function is sometimes responsible for building the completion string and sometimes it's the responsibility of the calling function.
Could we move this responsibility to one of the places (probably the callers?)


================
Comment at: lib/Sema/SemaCodeComplete.cpp:5168
+    // constructor.
+    if (const CXXRecordDecl *RD = Field->getType()->getAsCXXRecordDecl()) {
+      auto ConstResults = Results.GetConstructorResults(
----------------
NIT: use [[ https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code | early exits ]] to simplify code.
I.e.
```
// NOTE: Moved from the end of the loop.
SawLastInitializer = false;

const CXXRecordDecl *RD = Field->getType()->getAsCXXRecordDecl();
if (!RD) {
  CreatePrimitiveConstructor(Builder.getAllocator().CopyString(FieldName));
  AddField(Field);
  continue;
}
...


================
Comment at: lib/Sema/SemaCodeComplete.cpp:5169
+    if (const CXXRecordDecl *RD = Field->getType()->getAsCXXRecordDecl()) {
+      auto ConstResults = Results.GetConstructorResults(
+          RD, CodeCompletionResult(Field, SawLastInitializer
----------------
NIT: the `ConstResults` name is a bit confusing, because "const" has a another meaning in C++. Maybe rename to CtorResults and ConstructorResults?


================
Comment at: lib/Sema/SemaCodeComplete.cpp:5177
+            CodeCompletionContext::CCC_Symbol, Policy);
+        for (auto C : *CCS) {
+          switch (C.Kind) {
----------------
Could we pass the field name all the way down to the function that creates the original CodeCompletionString? (Instead of replacing the ctor name with a field name at the end)
This would mean less copies/allocations going around and would also be simpler to reason about (i.e. no "incomplete" code completion strings would be passed around).


================
Comment at: lib/Sema/SemaCodeComplete.cpp:5194
+    } else {
+      CreatePrimitiveConstructor(Builder.getAllocator().CopyString(FieldName));
+      AddField(Field);
----------------
Maybe show a type of the field for non-class types? They can typically be initialized from a single value of that type (with some exceptions, but showing the type shouldn't be confusing).
E.g. 
```
struct X {
  X() : ^ {} // could complete 'a({$1:int})`
  int a;
  int b;
};


Repository:
  rC Clang

https://reviews.llvm.org/D53654