Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add method overloading based on arity #18385

Closed
wants to merge 1 commit into from

Conversation

rmuir
Copy link
Contributor

@rmuir rmuir commented May 17, 2016

Currently painless only keys methods, constructors, static methods based on name. This means we cannot have multiple signatures with the same name, which makes it difficult to expand the whitelist.

For example new Date() vs new Date(long).

Here we just allow to overload based on number of parameters vs signatures: to keep everything still simple (including the user since much typing is dynamic and implicit conversions could get complex), but allow improvements for many cases where this is really needed.

For methods i added variants of String.indexOf, but in order to test the ctor and static method case I added a FeatureTest to the whitelist. This also contains getter/setters for loads and stores which we need to test but aren't yet exposed to the whitelist. It needs to be possible to divorce these two things, so we don't have to rush the api just to test.

We key methods on MethodKey (name + arity), but this only impacts the slow path for method lookups, the caching still works the same, so it does not have a performance impact.

@jdconrad
Copy link
Contributor

LGTM! Thanks @rmuir

@uschindler
Copy link
Contributor

uschindler commented May 17, 2016

LGTM.

About MethodKey: Maybe don't use the MethodKey class at all and instead do it like forbidden: Forbidden creates a pure String key (e.g. 'name@arity').

Thats just an idea.

@clintongormley clintongormley changed the title painless: add method overloading based on arity Add method overloading based on arity May 17, 2016
@rmuir
Copy link
Contributor Author

rmuir commented May 17, 2016

I don't think we should mix string concatenation here at all? I also don't plan to use tuple here, ever. that is horrible.

@rmuir rmuir closed this May 17, 2016
@uschindler
Copy link
Contributor

Sorry the Tuple was a suggestion. I will remove the comment, just for you!

@rmuir
Copy link
Contributor Author

rmuir commented May 17, 2016

I would rather painless not have this feature than use that horrible Tuple.

I am afraid if i commit this, that someone will refactor it to use Tuple later.

Quality not quantity.

@uschindler
Copy link
Contributor

I won't do that, trust me. My initial suggestion was to just use simple strings as key, but this involves more overhead in the fallback case (you need to first concat a string + the integer, vs just creating an object for the lookup in the map). So nuke this suggestion, too. For forbiddenapis its fine, but here the solution you brought is better.

rmuir added a commit that referenced this pull request May 17, 2016
Closes #18385

Squashed commit of the following:

commit b2819df4d392d69b86e5c96d358eb03424e67e02
Author: Robert Muir <rmuir@apache.org>
Date:   Tue May 17 09:15:47 2016 -0400

    add note about tuple

commit 85fcac6
Author: Robert Muir <rmuir@apache.org>
Date:   Mon May 16 23:39:25 2016 -0400

    painless: add method overloading based on arity
@uschindler
Copy link
Contributor

Thanks Robert for commiting! 💯

@jdconrad jdconrad mentioned this pull request May 17, 2016
18 tasks
@clintongormley clintongormley added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache and removed :Plugin Lang Painless labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >enhancement v5.0.0-alpha3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants