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

fixed JASSIST-267 (new ) #210

Merged
merged 2 commits into from Aug 31, 2018
Merged

Conversation

NingZhang-e
Copy link
Contributor

To fix JASSIST-267. Please kindly help review.

This is a replacement of #142 due to my organization suggest me use a new Github account.

@NingZhang-e NingZhang-e mentioned this pull request Jul 13, 2018
Copy link
Contributor

@nickl- nickl- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better than $142 nice one!

I would just revisit that funky for loop and make it more readable. Reduce the "What the?!?!" factor =)

Otherwise it seems you've hit the nail on the head, well done!

Note: I did not actually test the proposal.

int i = -1;
if (methodTypes.length == targetMethodTypes.length) {
for (Class<?> clz : methodTypes) {
i++;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is weird =/ why not just use a normal for statement with the i counter then?

Clean code according to comments.
@NingZhang-e
Copy link
Contributor Author

Code cleaned. Please check the new change.

@nickl-
Copy link
Contributor

nickl- commented Jul 13, 2018

Nice! Looks good! Will see if I can get a moment to test.

@NingZhang-e
Copy link
Contributor Author

@nickl- Do you have time checking it?

@nickl-
Copy link
Contributor

nickl- commented Aug 2, 2018

@NingZhang-Ericsson oh crap, I forgot about this but also haven't had a moment free to look at anything else.

You do know that I am not here at javassist in any official capacity, just to be clear. I don't mind helping out where I can but have commitments at other projects that takes priority. Besides @chibash is the one that needs to accept or reject this I can only say that I agree with you.

That said I did say I will test it for you so I will make some time to do that. Thank you for the reminder.

@nickl-
Copy link
Contributor

nickl- commented Aug 7, 2018

Tested and works as advertised.

Well done @NingZhang-Ericsson, nice one =)

RTBC
+1

@NingZhang-e
Copy link
Contributor Author

Great thanks to @nickl- !

@chibash Chiba-san, would you like to check with it?

@NingZhang-e
Copy link
Contributor Author

Hi,
@chibash, do you have any comment on this PR? Accept or reject it?
Your opinion is really needed.

@chibash
Copy link
Member

chibash commented Aug 22, 2018

Can you briefly explain differences between #142 and #267 with respect to how to fix JASSIST-267?

@NingZhang-e
Copy link
Contributor Author

#210
Change isOverloaded() to isDuplicated() is more precise and easier to understand. Because #267 is introduced by overloaded method. And it fixed the issue on the early stage as possible.
#142
I added one method called as isBridgeAndOverloadedAndPublic() which is a little confusing(too long method name), although it fixed #267, but it is a little hard to understand and maintain.

Hope I have explained the difference. :-)

@chibash chibash merged commit bca5c7a into jboss-javassist:master Aug 31, 2018
@chibash
Copy link
Member

chibash commented Aug 31, 2018

Thank you for fixing JASSIST-267.

@Elisedlund
Copy link

Elisedlund commented Aug 31, 2018

Nice!
We BTW open sourced our interception library that had issues with this bug. the library lets you manipulate existing objects and classes behavior runtime, It's achieving this by using javassist
example:

    //lambda on class
        SomeImpl obj = with(SomeImpl.class)
                .interceptAll(i -> {
                    System.out.println("before method: " + i.getMethodName() + " param: " + i.getParameter0());
                    return i.invoke();
                }).get();
        obj.log("123");
        //Console output:
        //        before method: log param: 123
        //        123


        //lambda on object
        SomeImpl obj2 = with(new SomeImpl())
                .interceptAll(i -> {
                    Object result = i.invoke();
                    System.out.println("after method: " + i.getMethodName() + " param: " + i.getParameter0());
                    return result;
                }).get();
        obj2.log("321");
        //Console output:
        //        321
        //        after method: log param: 321

lib: https://github.com/Ericsson/proxy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants