Skip to content

Fix #5135: Move well-known names to a new object WellKnownNames. #5136

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

Merged
merged 2 commits into from
Mar 10, 2025

Conversation

sjrd
Copy link
Member

@sjrd sjrd commented Mar 3, 2025

And related changes.

sjrd added 2 commits March 3, 2025 13:48
…mes`.

This breaks the circular dependency between the initialization of
`object Types` and `object Names`.
This also serves as a test that there is no deadlock between
`Names` and `Types` anymore. Without that fix, the addtion of
`NamesTest` triggered a deadlock in virtually every run of
`ir2_12/test`.
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.

Some comments on the last commit.

IMO it can also be split off into another PR so we can merge the first two quicker. Up to you.

* circular dependency between the `PrimTypeWithRef`s and their `PrimRef`s.
*/

def primRef: PrimRef = (charCode: @switch) match {
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered val primRef = PrimeRef(this) and then get the refs from the types for the aliases below?

case NullType => 'N'
case NothingType => 'E'
}
val charCode: Char = tpe.charCode
Copy link
Contributor

Choose a reason for hiding this comment

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

def?

case DoubleType => "double"
case NullType => "null"
case NothingType => "nothing"
val displayName: String = (tpe.charCode: @switch) match {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of the match, have you considered adding displayName as a private field to PrimTypeWithRef and just forward here?

That would essentially make PrimRef a "view" on a PrimType, which is maybe clearer overall?

@sjrd
Copy link
Member Author

sjrd commented Mar 9, 2025

IMO it can also be split off into another PR so we can merge the first two quicker. Up to you.

OK, I moved the 3rd commit to another branch https://github.com/sjrd/scala-js/tree/rearrange-init-sequence-of-primref . I'll turn it into a PR after this one is merged. It only contains the first two commits, now.

@sjrd sjrd requested a review from gzm0 March 9, 2025 22:19
@gzm0 gzm0 merged commit 2ed0b39 into scala-js:main Mar 10, 2025
3 checks passed
@sjrd sjrd deleted the fix-ir-names-types-deadlock branch March 10, 2025 07:25
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.

2 participants