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

Replace equality checks on sealed objects with is checks (fixes #129) #130

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

kyay10
Copy link
Contributor

@kyay10 kyay10 commented Apr 10, 2023

See #129 for the cause of the crash. This changes the code gen to do comparison checks on sealed objects using is instead of using equality. Also adds relevant tests.

@kyay10 kyay10 force-pushed the is-checks-for-comparison branch from d794e89 to 27998dd Compare April 10, 2023 21:31
@kyay10 kyay10 changed the title Add GenSealedEnum.useIsChecksForSealedObjectComparison (fixes #129) Replace equality checks on sealed objects with is checks (fixes #129) Apr 10, 2023
@kyay10
Copy link
Contributor Author

kyay10 commented Apr 10, 2023

Some weird errors are appearing. I'll investigate and see what's happening since I can't reproduce them locally.
Edit: Seems like the issue is related to the initialization ordering of SealedEnum.values, this can be fixed by making it a computed property, but I don't know if that's too costly.

We could also either make it by lazy, or by having a private val that stores a cached version of values (That might be the least compromise).

This also means that we can't use values inside of the init of a sealed class, but that's already the behavior of enum classes (see this simple playground example. Note that, if you remove the values()[] and just access the ordinal, there are no exceptions, so this PR brings the same constraints that enum class has in terms of initialization)

@alexvanyo
Copy link
Collaborator

+1 to matching the constraints of an enum class.

I think a lazy with a LazyThreadSafetyMode.PUBLICATION sounds good for the values with relatively little overhead.

@kyay10
Copy link
Contributor Author

kyay10 commented Apr 11, 2023

Is there any automated way to update the *fileName*Generated properties in the tests btw? I'm resorting to updating them manually, but with such a wide-reaching change, it takes a bit of time.

@kyay10 kyay10 force-pushed the is-checks-for-comparison branch from 27998dd to 281f1da Compare April 11, 2023 02:18
@kyay10
Copy link
Contributor Author

kyay10 commented Apr 11, 2023

I think a lazy with a LazyThreadSafetyMode.PUBLICATION sounds good for the values with relatively little overhead.

Ended up using that, and it seems to work pretty well. All tests are passing locally. Also moved the Flag tests to usecases. Hopefully, workflow tests should pass now

Copy link
Collaborator

@alexvanyo alexvanyo left a comment

Choose a reason for hiding this comment

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

Is there any automated way to update the fileNameGenerated properties in the tests btw? I'm resorting to updating them manually, but with such a wide-reaching change, it takes a bit of time.

Unfortunately no, I've just manually updated them as well.

@kyay10 kyay10 force-pushed the is-checks-for-comparison branch from 281f1da to 6a93d13 Compare April 11, 2023 18:22
@kyay10
Copy link
Contributor Author

kyay10 commented Apr 11, 2023

Is there any automated way to update the _fileName_Generated properties in the tests btw? I'm resorting to updating them manually, but with such a wide-reaching change, it takes a bit of time.

Unfortunately no, I've just manually updated them as well.

I've opened #131 to suggest an automatic way of doing that.

Copy link
Collaborator

@alexvanyo alexvanyo left a comment

Choose a reason for hiding this comment

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

Looks great to me, thank you for the change here and the contribution!

@alexvanyo alexvanyo merged commit f690fca into livefront:main Apr 11, 2023
@alexvanyo alexvanyo mentioned this pull request Jun 1, 2023
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