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

Updated the assert checking Dict versions #31110

Closed
wants to merge 1 commit into from

Conversation

eulerkochy
Copy link
Contributor

Changed the assert in code, to incorporate a check on versions of the Dict

@KristofferC
Copy link
Member

What was the reason you closed the previous PR https://github.com/JuliaLang/julia/pull/31097/files (that had the same changes as here)

Please try to keep working on the same PR instead of opening a new one. By reopening a new one the discussion on the earlier one is lost.

@eulerkochy
Copy link
Contributor Author

eulerkochy commented Feb 19, 2019

There were a lot of unwanted commits in that PR. I just tried to submit a 'clean' PR this time! Sorry for the inconvenience!
I'll take care of it from the next time!

@KristofferC
Copy link
Member

You can always update an existing PR no matter how bad you mess it up.

@vtjnash
Copy link
Member

vtjnash commented Feb 19, 2019

vtjnash: this a valid assert or should it be an error?

Is is a valid use of assert. If you follow the logic of the above function, it is logically impossible to happen, without memory corruption (however that could happen during race-y multithreading code—or equivalents such as finalizers and generator functions—hence the assert to kill the program).

@eulerkochy
Copy link
Contributor Author

error thrown is suggestive of such an event, at least it gives a reason why the program will get terminated due to mismatch of age of the Dict

@KristofferC
Copy link
Member

Yes but we want this to be an assert to allow it to potentially be removed in the future at some optimization level. That's the point of @assert over error.

@KristofferC
Copy link
Member

Closing since the assert is valid (#31110 (comment))

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