Add default values for function parameter chunks

[yvvan]

Summary

Append optional chunks with their default values. For example: before - "int i", after - "int i = 10"

[yvvan]

Support for non-simple types. Drop results with macros.

[yvvan]

This comment was removed by yvvan.

[yvvan]

Can someone check my review please?

[yvvan]

Anyone? :)

[klimek]

Can you give a bit more background what this is trying to do?

[yvvan]

In D33644#780188, @klimek wrote:

Can you give a bit more background what this is trying to do?

Sure :)
Currently default value string does not contain default value itself. This change fixes it and adds the default value to the string.

[klimek]

Ok, now I get it - can you please add tests? This is usually tested by adding a c-index-test based test.

lib/Sema/SemaCodeComplete.cpp

2411–2417 ↗

(On Diff #100705)

Can you use Lexer::getSourceText instead?

2453 ↗

(On Diff #100705)

Why the check for = in the PlaceholderStr?

[yvvan]

lib/Sema/SemaCodeComplete.cpp

2411–2417 ↗

(On Diff #100705)

If that does the same i will use it in the next diff update

2453 ↗

(On Diff #100705)

Not to add default value twice. If there's already "=" in placeholder string that means we've already added it in FormatFunctionParameter call

[klimek]

lib/Sema/SemaCodeComplete.cpp

2453 ↗

(On Diff #100705)

In which cases would the default value not be added in FormatFunctionParameter if there is one, and need to be added here?

[yvvan]

lib/Sema/SemaCodeComplete.cpp

2453 ↗

(On Diff #100705)

If Param->evaluateValue() can evaluate it it will set the default value in FormatFunctionParameter (that means it's a primitive type like "void func(int i = 0)").
In case it's some kind of non-primitive type like "void func(Foo foo = Foo(0, 0))" it will not be evaluated and we can use here the source manager to get the default value string. In this example it will be Foo(0, 0).

[yvvan]

Use Lexer::getSourceText

[klimek]

lib/Sema/SemaCodeComplete.cpp

2453 ↗

(On Diff #100705)

Why don't we always add it in the unevaluated form? I'd expect constants from macros are useful to see unevaluated?

[yvvan]

lib/Sema/SemaCodeComplete.cpp

2453 ↗

(On Diff #100705)

Hm, after using Lexer::getSourceText that makes sense! I had some issues getting source parts on my own without evaluation in some complicated cases with macros usage, etc.
I will test the solution without evaluation and update diff here if nothing breaks.

[yvvan]

Do not evaluate numbers.
Check for != "=" is needed not to mess with invalid default arguments or their types (without it I get "const Bar& bar = =" when Bar is not defined)

[yvvan]

Recheck please the latest diff

[klimek]

In D33644#783903, @yvvan wrote:

Do not evaluate numbers.
Check for != "=" is needed not to mess with invalid default arguments or their types (without it I get "const Bar& bar = =" when Bar is not defined)

Shouldn't we than instead check that error case?

[yvvan]

In D33644#793573, @klimek wrote:

In D33644#783903, @yvvan wrote:

Do not evaluate numbers.
Check for != "=" is needed not to mess with invalid default arguments or their types (without it I get "const Bar& bar = =" when Bar is not defined)

Shouldn't we than instead check that error case?

I don't know the proper way to do that :) I also don't know the full amount of errors that might cause such behavior.
You can suggest the solution if you have some idea. Current one is safe because we just ignore any case that causes empty default string.

[klimek]

In D33644#793577, @yvvan wrote:

In D33644#793573, @klimek wrote:

In D33644#783903, @yvvan wrote:

Do not evaluate numbers.
Check for != "=" is needed not to mess with invalid default arguments or their types (without it I get "const Bar& bar = =" when Bar is not defined)

Shouldn't we than instead check that error case?

I don't know the proper way to do that :) I also don't know the full amount of errors that might cause such behavior.
You can suggest the solution if you have some idea. Current one is safe because we just ignore any case that causes empty default string.

Taking your example "const Bar& bar = =" when Bar is not defined:
What happens now? Will it be "const Bar& bar ="? I argue that is not better than "const Bar& bar = =".
Btw, can you add tests? I think that will make it easier to see what's actually happening.

[yvvan]

In D33644#793594, @klimek wrote:

In D33644#793577, @yvvan wrote:

In D33644#793573, @klimek wrote:

In D33644#783903, @yvvan wrote:

Do not evaluate numbers.
Check for != "=" is needed not to mess with invalid default arguments or their types (without it I get "const Bar& bar = =" when Bar is not defined)

Shouldn't we than instead check that error case?

I don't know the proper way to do that :) I also don't know the full amount of errors that might cause such behavior.
You can suggest the solution if you have some idea. Current one is safe because we just ignore any case that causes empty default string.

Taking your example "const Bar& bar = =" when Bar is not defined:
What happens now? Will it be "const Bar& bar ="? I argue that is not better than "const Bar& bar = =".
Btw, can you add tests? I think that will make it easier to see what's actually happening.

It will be just "const Bar& bar" in that case with this patch applied. So it is better than both "const Bar& bar =" and "const Bar& bar = =" :)
About tests - I agree that it's nice to have them. Can I find something similar in the current test set (as an example) and where is it better to put my tests?

[klimek]

In D33644#793601, @yvvan wrote:

In D33644#793594, @klimek wrote:

In D33644#793577, @yvvan wrote:

In D33644#793573, @klimek wrote:

In D33644#783903, @yvvan wrote:

Do not evaluate numbers.
Check for != "=" is needed not to mess with invalid default arguments or their types (without it I get "const Bar& bar = =" when Bar is not defined)

Shouldn't we than instead check that error case?

I don't know the proper way to do that :) I also don't know the full amount of errors that might cause such behavior.
You can suggest the solution if you have some idea. Current one is safe because we just ignore any case that causes empty default string.

Taking your example "const Bar& bar = =" when Bar is not defined:
What happens now? Will it be "const Bar& bar ="? I argue that is not better than "const Bar& bar = =".
Btw, can you add tests? I think that will make it easier to see what's actually happening.

It will be just "const Bar& bar" in that case with this patch applied. So it is better than both "const Bar& bar =" and "const Bar& bar = =" :)
About tests - I agree that it's nice to have them. Can I find something similar in the current test set (as an example) and where is it better to put my tests?

All of test/Index/complete-* basically test this, I think. Try to find a good spot or write a new test.

[yvvan]

Add tests, append tests with default values. Move all default value handling to GetDefaultValueString

[yvvan]

Ping :)
It's quite likely ok now because covered with tests. Check please.

[klimek]

lib/Sema/SemaCodeComplete.cpp

2411–2412 ↗

(On Diff #105045)

Can you expand in the comment on when each of these happens? Intuitively I'd have expected that only one of these cases can happen (I assume this is because of the forward-declared type?)

[yvvan]

Add comments why it's needed to add '=' to default value in some cases

[yvvan]

Ping! Comments are added :)

[yvvan]

Add more checks into GetDefaultValueString to make it safe

[klimek]

+Richard, as apparently we get the source ranges for default values of built-in types without the preceding "=", but for user-defined types including the preceding "=" - is that intentional?

lib/Sema/SemaCodeComplete.cpp

2424 ↗

(On Diff #106170)

"The Lexer returns default-values of built-in types without '=', and default-value of user-defined types with it."

But apart from that: that seems like a bug in the range of the default value? I'm now really confused :)

[yvvan]

So do we wait until the '=' case is more clear? This change should not break anything if it's fixed in either direction (if '=' will be provided always or never)

[klimek]

In D33644#812693, @yvvan wrote:

So do we wait until the '=' case is more clear? This change should not break anything if it's fixed in either direction (if '=' will be provided always or never)

Ok, if you add a TODO I think this is fine for now.

[yvvan]

Add TODO about '=' check

[klimek]

lg

lib/Sema/SemaCodeComplete.cpp

2422 ↗

(On Diff #107077)

Final nit: TODO is spelled FIXME in llvm/clang.

cfe/trunk/lib/Sema/SemaCodeComplete.cpp

if (SuppressBlock) {
 
     if (!SuppressBlockName && BlockDecl->getIdentifier())
       Result += BlockDecl->getIdentifier()->getName();
   }
 
   return Result;
 }
 
+static std::string GetDefaultValueString(const ParmVarDecl *Param,
+                                         const SourceManager &SM,
+                                         const LangOptions &LangOpts) {
+  const Expr *defaultArg = Param->getDefaultArg();
+  if (!defaultArg)
+    return "";
+  const SourceRange SrcRange = defaultArg->getSourceRange();
+  CharSourceRange CharSrcRange = CharSourceRange::getTokenRange(SrcRange);
+  bool Invalid = CharSrcRange.isInvalid();
+  if (Invalid)
+    return "";
+  StringRef srcText = Lexer::getSourceText(CharSrcRange, SM, LangOpts, &Invalid);
+  if (Invalid)
+    return "";
+
+  if (srcText.empty() || srcText == "=") {
+    // Lexer can't determine the value.
+    // This happens if the code is incorrect (for example class is forward declared).
+    return "";
+  }
+  std::string DefValue{srcText};
+  // FIXME: remove this check if the Lexer::getSourceText value is fixed and
+  // this value always has (or always does not have) '=' in front of it
+  if (DefValue.at(0) != '=') {
+    // If we don't have '=' in front of value.
+    // Lexer returns built-in types values without '=' and user-defined types values with it.
+    return " = " + DefValue;
+  }
+  return " " + DefValue;
+}
+
 /// \brief Add function parameter chunks to the given code completion string.
 static void AddFunctionParameterChunks(Preprocessor &PP,
                                        const PrintingPolicy &Policy,
                                        const FunctionDecl *Function,
                                        CodeCompletionBuilder &Result,
                                        unsigned Start = 0,
                                        bool InOptional = false) {
   bool FirstParameter = true;
if (FirstParameter)
       FirstParameter = false;
     else
       Result.AddChunk(CodeCompletionString::CK_Comma);
     
     InOptional = false;
     
     // Format the placeholder string.
     std::string PlaceholderStr = FormatFunctionParameter(Policy, Param);
+    if (Param->hasDefaultArg())
+      PlaceholderStr += GetDefaultValueString(Param, PP.getSourceManager(), PP.getLangOpts());
 
     if (Function->isVariadic() && P == N - 1)
       PlaceholderStr += ", ...";
 
     // Add the placeholder string.
     Result.AddPlaceholderChunk(
                              Result.getAllocator().CopyString(PlaceholderStr));
   }
if (FirstParameter)
       FirstParameter = false;
     else
       Result.AddChunk(CodeCompletionString::CK_Comma);
 
     InOptional = false;
 
     // Format the placeholder string.
     std::string Placeholder;
-    if (Function)
-      Placeholder = FormatFunctionParameter(Policy, Function->getParamDecl(P));
-    else
+    if (Function) {
+      const ParmVarDecl *Param = Function->getParamDecl(P);
+      Placeholder = FormatFunctionParameter(Policy, Param);
+      if (Param->hasDefaultArg())
+        Placeholder += GetDefaultValueString(Param, Context.getSourceManager(), Context.getLangOpts());
+    } else {
       Placeholder = Prototype->getParamType(P).getAsString(Policy);
+    }
 
     if (P == CurrentArg)
       Result.AddCurrentParameterChunk(
         Result.getAllocator().CopyString(Placeholder));
     else
       Result.AddPlaceholderChunk(Result.getAllocator().CopyString(Placeholder));
   }
 

cfe/trunk/test/CodeCompletion/functions.cpp

 void f(int i, int j = 2, int k = 5);
 void f(float x, float y...);
        
 void test() {
   ::
   // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:5:5 %s -o - | FileCheck -check-prefix=CHECK-CC1 %s
-  // CHECK-CC1: f(<#int i#>{#, <#int j#>{#, <#int k#>#}#})
+  // CHECK-CC1: f(<#int i#>{#, <#int j = 2#>{#, <#int k = 5#>#}#})
   // CHECK-CC1: f(<#float x#>, <#float y, ...#>)

cfe/trunk/test/Index/code-completion.cpp

 
 Z::operator int() const {
   return 0;
 }
 
 // CHECK-MEMBER: FieldDecl:{ResultType double}{TypedText member}
 // CHECK-MEMBER: FieldDecl:{ResultType int}{Text X::}{TypedText member}
 // CHECK-MEMBER: FieldDecl:{ResultType float}{Text Y::}{TypedText member}
-// CHECK-MEMBER: CXXMethod:{ResultType void}{Informative Y::}{TypedText memfunc}{LeftParen (}{Optional {Placeholder int i}}{RightParen )}
+// CHECK-MEMBER: CXXMethod:{ResultType void}{Informative Y::}{TypedText memfunc}{LeftParen (}{Optional {Placeholder int i = 17}}{RightParen )}
 // CHECK-MEMBER: CXXConversion:{TypedText operator int}{LeftParen (}{RightParen )}{Informative  const}
 // CHECK-MEMBER: CXXMethod:{ResultType Z &}{TypedText operator=}{LeftParen (}{Placeholder const Z &}{RightParen )}
 // CHECK-MEMBER: CXXMethod:{ResultType X &}{Text X::}{TypedText operator=}{LeftParen (}{Placeholder const X &}{RightParen )}
 // CHECK-MEMBER: CXXMethod:{ResultType Y &}{Text Y::}{TypedText operator=}{LeftParen (}{Placeholder const Y &}{RightParen )}
 // CHECK-MEMBER: EnumConstantDecl:{ResultType X::E}{Informative E::}{TypedText Val1}
 // CHECK-MEMBER: StructDecl:{TypedText X}{Text ::}
 // CHECK-MEMBER: StructDecl:{TypedText Y}{Text ::}
 // CHECK-MEMBER: StructDecl:{TypedText Z}{Text ::}

 // CHECK-OVERLOAD-NEXT: Objective-C interface
 
 // RUN: c-index-test -code-completion-at=%s:37:10 %s | FileCheck -check-prefix=CHECK-EXPR %s
 // CHECK-EXPR: NotImplemented:{TypedText int} (50)
 // CHECK-EXPR: NotImplemented:{TypedText long} (50)
 // CHECK-EXPR: FieldDecl:{ResultType double}{TypedText member} (17)
 // CHECK-EXPR: FieldDecl:{ResultType int}{Text X::}{TypedText member} (9)
 // CHECK-EXPR: FieldDecl:{ResultType float}{Text Y::}{TypedText member} (18)
-// CHECK-EXPR: CXXMethod:{ResultType void}{TypedText memfunc}{LeftParen (}{Optional {Placeholder int i}}{RightParen )} (37)
+// CHECK-EXPR: CXXMethod:{ResultType void}{TypedText memfunc}{LeftParen (}{Optional {Placeholder int i = 17}}{RightParen )} (37)
 // CHECK-EXPR: Namespace:{TypedText N}{Text ::} (75)
 // CHECK-EXPR: Completion contexts:
 // CHECK-EXPR-NEXT: Any type
 // CHECK-EXPR-NEXT: Any value
 // CHECK-EXPR-NEXT: Enum tag
 // CHECK-EXPR-NEXT: Union tag
 // CHECK-EXPR-NEXT: Struct tag
 // CHECK-EXPR-NEXT: Class name
 // CHECK-EXPR-NEXT: Nested name specifier
 // CHECK-EXPR-NEXT: Objective-C interface

cfe/trunk/test/Index/complete-optional-params.cpp

 // Note: the run lines follow their respective tests, since line/column
 // matter in this test.
 
 void foo(int a = 42, int = 42);
 void bar(int a, int b = 42, int c = 42);
 void baz(int a = 42, ...);
 struct S{ S(int a = 42, int = 42) {} };
 
+class Bar1 { public: Bar1() {} }; class Bar2;
+void foo_2(Bar1 b1 = Bar1(), Bar2 b2 = Bar2());
+
 int main() {
     foo(42, 42);
     bar(42, 42, 42);
     baz(42, 42, 42);
     S s(42, 42);
+    foo_2();
 }
 
-// RUN: c-index-test -code-completion-at=%s:10:9 %s | FileCheck -check-prefix=CHECK-CC1 %s
-// CHECK-CC1: OverloadCandidate:{ResultType void}{Text foo}{LeftParen (}{Optional {CurrentParameter int a}{Optional {Comma , }{Placeholder int}}}{RightParen )} (1)
+// RUN: c-index-test -code-completion-at=%s:13:9 %s | FileCheck -check-prefix=CHECK-CC1 %s
+// CHECK-CC1: OverloadCandidate:{ResultType void}{Text foo}{LeftParen (}{Optional {CurrentParameter int a = 42}{Optional {Comma , }{Placeholder int = 42}}}{RightParen )} (1)
 // CHECK-CC1: Completion contexts:
 // CHECK-CC1-NEXT: Any type
 // CHECK-CC1-NEXT: Any value
 // CHECK-CC1-NEXT: Enum tag
 // CHECK-CC1-NEXT: Union tag
 // CHECK-CC1-NEXT: Struct tag
 // CHECK-CC1-NEXT: Class name
 // CHECK-CC1-NEXT: Nested name specifier
 // CHECK-CC1-NEXT: Objective-C interface
 
-// RUN: c-index-test -code-completion-at=%s:11:9 %s | FileCheck -check-prefix=CHECK-CC2 %s
-// CHECK-CC2: OverloadCandidate:{ResultType void}{Text bar}{LeftParen (}{CurrentParameter int a}{Optional {Comma , }{Placeholder int b}{Optional {Comma , }{Placeholder int c}}}{RightParen )} (1)
+// RUN: c-index-test -code-completion-at=%s:14:9 %s | FileCheck -check-prefix=CHECK-CC2 %s
+// CHECK-CC2: OverloadCandidate:{ResultType void}{Text bar}{LeftParen (}{CurrentParameter int a}{Optional {Comma , }{Placeholder int b = 42}{Optional {Comma , }{Placeholder int c = 42}}}{RightParen )} (1)
 // CHECK-CC2: Completion contexts:
 // CHECK-CC2-NEXT: Any type
 // CHECK-CC2-NEXT: Any value
 // CHECK-CC2-NEXT: Enum tag
 // CHECK-CC2-NEXT: Union tag
 // CHECK-CC2-NEXT: Struct tag
 // CHECK-CC2-NEXT: Class name
 // CHECK-CC2-NEXT: Nested name specifier
 // CHECK-CC2-NEXT: Objective-C interface
 
-// RUN: c-index-test -code-completion-at=%s:11:16 %s | FileCheck -check-prefix=CHECK-CC3 %s
-// CHECK-CC3: OverloadCandidate:{ResultType void}{Text bar}{LeftParen (}{Placeholder int a}{Optional {Comma , }{Placeholder int b}{Optional {Comma , }{CurrentParameter int c}}}{RightParen )} (1)
+// RUN: c-index-test -code-completion-at=%s:14:16 %s | FileCheck -check-prefix=CHECK-CC3 %s
+// CHECK-CC3: OverloadCandidate:{ResultType void}{Text bar}{LeftParen (}{Placeholder int a}{Optional {Comma , }{Placeholder int b = 42}{Optional {Comma , }{CurrentParameter int c = 42}}}{RightParen )} (1)
 // CHECK-CC3: Completion contexts:
 // CHECK-CC3-NEXT: Any type
 // CHECK-CC3-NEXT: Any value
 // CHECK-CC3-NEXT: Enum tag
 // CHECK-CC3-NEXT: Union tag
 // CHECK-CC3-NEXT: Struct tag
 // CHECK-CC3-NEXT: Class name
 // CHECK-CC3-NEXT: Nested name specifier
 // CHECK-CC3-NEXT: Objective-C interface
 
-// RUN: c-index-test -code-completion-at=%s:12:16 %s | FileCheck -check-prefix=CHECK-CC4 %s
-// CHECK-CC4: OverloadCandidate:{ResultType void}{Text baz}{LeftParen (}{Optional {Placeholder int a}{Optional {Comma , }{CurrentParameter ...}}}{RightParen )} (1)
+// RUN: c-index-test -code-completion-at=%s:15:16 %s | FileCheck -check-prefix=CHECK-CC4 %s
+// CHECK-CC4: OverloadCandidate:{ResultType void}{Text baz}{LeftParen (}{Optional {Placeholder int a = 42}{Optional {Comma , }{CurrentParameter ...}}}{RightParen )} (1)
 // CHECK-CC4: Completion contexts:
 // CHECK-CC4-NEXT: Any type
 // CHECK-CC4-NEXT: Any value
 // CHECK-CC4-NEXT: Enum tag
 // CHECK-CC4-NEXT: Union tag
 // CHECK-CC4-NEXT: Struct tag
 // CHECK-CC4-NEXT: Class name
 // CHECK-CC4-NEXT: Nested name specifier
 // CHECK-CC4-NEXT: Objective-C interface
 
-// RUN: c-index-test -code-completion-at=%s:13:9 %s | FileCheck -check-prefix=CHECK-CC5 %s
-// CHECK-CC5: OverloadCandidate:{Text S}{LeftParen (}{Optional {CurrentParameter int a}{Optional {Comma , }{Placeholder int}}}{RightParen )} (1)
+// RUN: c-index-test -code-completion-at=%s:16:9 %s | FileCheck -check-prefix=CHECK-CC5 %s
+// CHECK-CC5: OverloadCandidate:{Text S}{LeftParen (}{Optional {CurrentParameter int a = 42}{Optional {Comma , }{Placeholder int = 42}}}{RightParen )} (1)
 // CHECK-CC5: OverloadCandidate:{Text S}{LeftParen (}{CurrentParameter const S &}{RightParen )} (1)
 // CHECK-CC5: Completion contexts:
 // CHECK-CC5-NEXT: Any type
 // CHECK-CC5-NEXT: Any value
 // CHECK-CC5-NEXT: Enum tag
 // CHECK-CC5-NEXT: Union tag
 // CHECK-CC5-NEXT: Struct tag
 // CHECK-CC5-NEXT: Class name
 // CHECK-CC5-NEXT: Nested name specifier
 // CHECK-CC5-NEXT: Objective-C interface
+
+// RUN: c-index-test -code-completion-at=%s:17:11 %s | FileCheck -check-prefix=CHECK-CC6 %s
+// CHECK-CC6: FunctionDecl:{ResultType void}{TypedText foo_2}{LeftParen (}{Optional {Placeholder Bar1 b1 = Bar1()}{Optional {Comma , }{Placeholder Bar2 b2}}}{RightParen )} (50)
+// CHECK-CC6: Completion contexts:
+// CHECK-CC6-NEXT: Any type
+// CHECK-CC6-NEXT: Any value
+// CHECK-CC6-NEXT: Enum tag
+// CHECK-CC6-NEXT: Union tag
+// CHECK-CC6-NEXT: Struct tag
+// CHECK-CC6-NEXT: Class name
+// CHECK-CC6-NEXT: Nested name specifier
+// CHECK-CC6-NEXT: Objective-C interface