8000 Refactoring: Isolate handling of javalib methods with special bodies. by sjrd · Pull Request #5173 · scala-js/scala-js · GitHub
[go: up one dir, main page]
Skip to content

Refactoring: Isolate handling of javalib methods with special bodies. #5173

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 19, 2025
Merged

Refactoring: Isolate handling of javalib methods with special bodies. #5173

merged 1 commit into from
May 19, 2025
+106 −73

Conversation

sjrd
Copy link
Member
@sjrd sjrd commented May 19, 2025

A number of methods from the javalib are special-cased by the compiler, which replaces their body with a dedicated UnaryOp or BinaryOp.

This commit refactors that handling to isolate it better from the handling of regular methods. We also make it a bit more flexible, so that we can more easily add further such methods in the future.

Extracted from several ongoing PRs. Comments from #5158:

46962c5#r2094545568

Since we are touching this: Is there a reason we do this based on generated class name and not based on the Scalac symbol of the method? Feels odd (if there is a reason, it should probably go here as comment).

Mostly for convenience. I added a comment on the definition of JavalibMethodsWithOpBody.

46962c5#r2094546049

I feel the self-matching style here is odd, since JavalibOpBody is a single-purpose type. Any reason not to just use an abstract method?

I switched to an abstract method, but I'm not sure it's more readable.

@sjrd
Refactoring: Isolate handling of javalib methods with special bodies.
5c31843 
A number of methods from the javalib are special-cased by the
compiler, which replaces their body with a dedicated `UnaryOp` or
`BinaryOp`.

This commit refactors that handling to isolate it better from the
handling of regular methods. We also make it a bit more flexible,
so that we can more easily add further such methods in the future.
@sjrd sjrd requested a review from gzm0 May 19, 2025 09:21
gzm0
gzm0 approved these changes May 19, 2025
View reviewed changes
Copy link
Contributor
@gzm0 gzm0 left a comment

Choose a reason for hiding this comment

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

LGTM. I agree that using an abstract method does not improve readability, but IMO it keeps related logic closer together. So I would stick with it.

@sjrd
Copy link
Member Author
sjrd commented May 19, 2025

OK, let's stick with it, then. I have no strong feeling either way.

@sjrd sjrd merged commit 2c17a60 into scala-js:main May 19, 2025
3 checks passed
@sjrd sjrd deleted the refactor-javalib-special-methods-codegen branch May 19, 2025 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gzm0 gzm0 gzm0 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sjrd @gzm0
0