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

Ensure to call jl_islayout_inline() even in the case @assert is deactivated #30615

Merged
merged 1 commit into from
Jan 6, 2019
Merged

Ensure to call jl_islayout_inline() even in the case @assert is deactivated #30615

merged 1 commit into from
Jan 6, 2019

Conversation

petershintech
Copy link
Contributor

No description provided.

@yuyichao
Copy link
Contributor

yuyichao commented Jan 6, 2019

Why?

Edit: Nvm, I missed the argument..

@ararslan ararslan requested a review from vtjnash January 6, 2019 00:55
@ararslan
Copy link
Member

ararslan commented Jan 6, 2019

Thanks for the contribution @petershintech! It would be good to know what the justification for the change is. Could you provide some more information?

@jlapeyre
Copy link
Contributor

jlapeyre commented Jan 6, 2019

See this thread: https://discourse.julialang.org/t/no-problem-with-more-than-500-assert-statements-in-v1-0-3/19255

I don't think this PR offers an advantage at the moment. But, if there is a mechanism to turn off assertions in the future, then the code will fail without this PR.

@KristofferC
Copy link
Member

This is the docs for @assert:

│ Warning

│ An assert might be disabled at various optimization levels. Assert should therefore only be used as a
│ debugging tool and not used for authentication verification (e.g., verifying passwords), nor should
│ side effects needed for the function to work correctly be used inside of asserts.

So this LGTM.

@petershintech
Copy link
Contributor Author

Yes. The problem is, this code depends on the side effect of the @Assert statement to return a correct result.

@ararslan ararslan merged commit 2931072 into JuliaLang:master Jan 6, 2019
KristofferC pushed a commit that referenced this pull request Jan 11, 2019
@KristofferC KristofferC mentioned this pull request Jan 11, 2019
53 tasks
@StefanKarpinski StefanKarpinski added triage This should be discussed on a triage call backport 1.1 and removed backport 1.0 triage This should be discussed on a triage call labels Jan 31, 2019
@mbauman mbauman changed the title Ensure to call jl_islayout_inline() even in the case @aasert is deactivated Ensure to call jl_islayout_inline() even in the case @assert is deactivated Jan 31, 2019
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.

7 participants