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

Remove -Xcheckinit from the default option set #85

Merged
merged 2 commits into from
Jul 19, 2022

Conversation

DavidGregory084
Copy link
Member

@DavidGregory084 DavidGregory084 commented Jul 18, 2022

Resolves #83

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Thanks. I do feel bad that this goes against Rob's wishes in #10 but I'm inclined to follow the compiler engineers' advice that this isn't intended for production :)


### Changed

* [#85](https://github.com/typelevel/sbt-tpolecat/pull/85) - `-Xcheckinit` was removed from the default option set. After discussion on issues [#10](https://github.com/typelevel/sbt-tpolecat/issues/10) and [#83](https://github.com/typelevel/sbt-tpolecat/issues/83) it has become apparent that although `-Xcheckinit` offers a lot of value for some users, it creates some very tricky problems for users that are writing async code. The initialization checking code this option produces introduces `@volatile` variables that can disguise memory visibility issues.
Copy link
Member

Choose a reason for hiding this comment

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

Technically, as long as you test and publish your code with -Xcheckinit then you won't have hidden memory visibility bugs, just performance issues. It's only a problem if you use it for testing, but then publish without it (removing all the extra memory barriers).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that is true, I wonder how it affects things when (for example) a user specifies -Xcheckinit and has inlining enabled. Seems like there are a lot of possible interactions here depending upon the permutations of user vs library-specified options.

@DavidGregory084 DavidGregory084 merged commit e3cec9d into main Jul 19, 2022
@DavidGregory084 DavidGregory084 deleted the remove-checkinit branch July 21, 2022 16:12
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.

-Xcheckinit should not be enabled by default
2 participants