Skip to content

fix #33370, avoid collisions between gensyms and anon function names #33426

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

Merged
merged 1 commit into from
Oct 3, 2019

Conversation

JeffBezanson
Copy link
Member

Alternate fix for #33370.

@JeffBezanson JeffBezanson added bugfix This change fixes an existing bug backport 1.3 labels Sep 30, 2019
@JeffBezanson JeffBezanson force-pushed the jb/fix33370 branch 5 times, most recently from 34874de to c9ea2c6 Compare October 1, 2019 18:44
@JeffBezanson JeffBezanson added this to the 1.3 milestone Oct 1, 2019
Copy link
Member

@c42f c42f left a comment

Choose a reason for hiding this comment

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

Looks reasonable enough to me. It would be nice if disparate parts of the code didn't communicate so intimately via naming conventions, but that's a separate issue I think.

# issue #33370
abstract type B33370 end

let n = gensym(), c(x) = B33370[x][1]()
Copy link
Member

Choose a reason for hiding this comment

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

For some reason I found the intent of this typed array very confusing at first sight. I think I would have been less confused by c(x) = Ref{B33370}(x)[](). Especially with a comment saying that this is to prevent type inference from determining the concrete type of x and to force a runtime method table lookup. (at least I assume this is roughly the idea.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the idea is to force inference to use the abstract type in its method lookup. At some point we should change all such things in our tests to something like infer_as(x, T).

@KristofferC KristofferC merged commit 888d32a into master Oct 3, 2019
@KristofferC KristofferC deleted the jb/fix33370 branch October 3, 2019 08:42
KristofferC pushed a commit that referenced this pull request Oct 3, 2019
@KristofferC KristofferC mentioned this pull request Oct 3, 2019
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants