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

Don't use internal jl_set_const to create constants #125

Merged
merged 1 commit into from
Jan 26, 2025

Conversation

Keno
Copy link
Member

@Keno Keno commented Jan 25, 2025

The internal function jl_set_const is allowed during bootstrap only and ignores world age partition. This would give incorrect results after JuliaLang/julia#57150. Just eval the constant definition directly, which has well defined semantics.

Copy link

codecov bot commented Jan 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.19%. Comparing base (1c7eb92) to head (52d5703).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #125   +/-   ##
=======================================
  Coverage   79.19%   79.19%           
=======================================
  Files          10       10           
  Lines        1923     1923           
=======================================
  Hits         1523     1523           
  Misses        400      400           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DilumAluthge
Copy link
Member

It would be nice to get this reviewed and merge relatively quickly, so that we can unblock JuliaLang/julia#57150.

Copy link
Collaborator

@JamesWrigley JamesWrigley left a comment

Choose a reason for hiding this comment

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

LGTM! However, I would wait for #122 to merged before bumping the Distributed version in Julia because that has some thread-safety fixes which are necessary since #101. I'll have a final look at #122 later today.

EDIT: #122 has been merged, feel free to merge this and bump the version.

@JamesWrigley
Copy link
Collaborator

I took the liberty of rebasing so it can be tested with multiple threads.

The internal function `jl_set_const` is allowed during bootstrap only
and ignores world age partition. This would give incorrect results after
JuliaLang/julia#57150. Just eval the constant definition directly,
which has well defined semantics.
@JamesWrigley JamesWrigley merged commit 0887df4 into master Jan 26, 2025
8 checks passed
@JamesWrigley JamesWrigley deleted the kf/jlsetconst branch January 26, 2025 15:26
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.

3 participants