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

[jvm] Put all @Override in place #639

Merged
merged 3 commits into from May 27, 2020
Merged

[jvm] Put all @Override in place #639

merged 3 commits into from May 27, 2020

Conversation

andreoss
Copy link
Contributor

Add @Override on overriden methods, remove public modifier in interfaces, also remove boxing and use diamond where applicable.

@andreoss andreoss marked this pull request as ready for review May 23, 2020 17:48
@usev6
Copy link
Contributor

usev6 commented May 25, 2020

I've run a spectest with this changes and didn't see any fallout.

I'm not a Java developer, so I don't dare to do a final assessment here. That being said, adding those @Override and using the lambda syntax for Threads look good (and uncontroversial) to me. (I'd hit the merge button for those changes.)

What I'm not completely sure about are the removals of a couple of static/public/static public keywords and the removal of boxing (8dc6af1). I guess, those are fine too, but don't fully understand the effects of these changes and I don't know why we had the old version in the first place. Also I remember #507, that was submitted to clean up the Java code. It ended up being reverted and earned an unhappy comment from @jnthn about mainly making git blame less useful (which I can confirm ;)). I can't imagine that the current PR would get the same assessment, but still ...

I want to add that I'm very happy to see efforts to improve/cleanup/revive the JVM backend. Thanks a lot, @andreoss .

@andreoss
Copy link
Contributor Author

What I'm not completely sure about are the removals of a couple of static/public/static public keywords and the removal of boxing (8dc6af1).

In the cases I found the modifiers were redundant, such as public for an interface method, and static for an inner interface.
And while auto-boxing has some drawbacks, I doubt the explicit boxing was indented here (or may be some very early rakudo/nqp versions had to run on prior-1.5 JVM).
Same goes with diamond operator and lambdas.

Copy link
Contributor

@jnthn jnthn left a comment

Choose a reason for hiding this comment

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

This looks sensible to me; thanks.

@jnthn jnthn merged commit c2f40b4 into Raku:master May 27, 2020
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

3 participants