Page MenuHomePhabricator

Add default values for function parameter chunks
ClosedPublic

Authored by yvvan on May 29 2017, 5:55 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

yvvan created this revision.May 29 2017, 5:55 AM
yvvan updated this revision to Diff 100705.May 30 2017, 7:18 AM
yvvan edited the summary of this revision. (Show Details)

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

yvvan added a comment.Jun 6 2017, 3:27 AM
This comment was removed by yvvan.

Can someone check my review please?

yvvan added a comment.Jun 14 2017, 1:53 AM

Anyone? :)

klimek edited edge metadata.Jun 14 2017, 4:41 AM

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

yvvan added a comment.Jun 14 2017, 5:58 AM

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.

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 added inline comments.Jun 14 2017, 7:39 AM
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 added inline comments.Jun 14 2017, 8:58 AM
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 added inline comments.Jun 14 2017, 10:09 AM
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 updated this revision to Diff 102800.Jun 16 2017, 3:21 AM

Use Lexer::getSourceText

klimek added inline comments.Jun 19 2017, 2:10 AM
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 added inline comments.Jun 19 2017, 2:16 AM
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 updated this revision to Diff 103016.Jun 19 2017, 4:51 AM

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 added a comment.Jun 27 2017, 4:08 AM

Recheck please the latest diff

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 added a comment.Jun 28 2017, 3:50 AM

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.

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 added a comment.Jun 28 2017, 4:40 AM

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?

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 updated this revision to Diff 105045.Jul 3 2017, 3:44 AM

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

yvvan added a comment.Jul 10 2017, 7:33 AM

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

klimek added inline comments.Jul 10 2017, 8:43 AM
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 updated this revision to Diff 105976.Jul 11 2017, 1:55 AM

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

yvvan added a comment.Jul 12 2017, 2:34 AM

Ping! Comments are added :)

yvvan updated this revision to Diff 106170.Jul 12 2017, 4:18 AM

Add more checks into GetDefaultValueString to make it safe

+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 added a comment.Jul 18 2017, 4:59 AM

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)

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 updated this revision to Diff 107077.Jul 18 2017, 5:17 AM

Add TODO about '=' check

klimek accepted this revision.Jul 18 2017, 5:30 AM

lg

lib/Sema/SemaCodeComplete.cpp
2422 ↗(On Diff #107077)

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

This revision is now accepted and ready to land.Jul 18 2017, 5:30 AM
This revision was automatically updated to reflect the committed changes.