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 show(::IO, ::Type{StaticArrays.Blah}) overloads #769

Merged
merged 1 commit into from
Apr 22, 2020

Conversation

c42f
Copy link
Member

@c42f c42f commented Apr 22, 2020

Apparently these can be problematic for compiler-internal reasons, and I
think the printing has been improved such that the remaining cases of
these may not do anything useful. So let's just remove them.

Apparently these can be problematic for compiler-internal reasons, and I
think the printing has been improved such that the remaining cases of
these may not do anything useful. So let's just remove them.
@c42f c42f force-pushed the cjf/remove-show-Type branch from 71cf83c to 00a6198 Compare April 22, 2020 13:16
@c42f
Copy link
Member Author

c42f commented Apr 22, 2020

By the way, I'd say exactly why removing these is a good idea if I understood it! All I really know is that some obscure and very nasty Pumas.jl issue may have been related to the presence of these methods. And that removing them would do no real harm. CC @andreasnoack

@c42f
Copy link
Member Author

c42f commented Apr 22, 2020

See also PainterQubits/Unitful.jl#321

@timholy
Copy link
Member

timholy commented Apr 22, 2020

Old history here: JuliaLang/julia#13306. Not sure how much of a modern problem it is, but I've been paranoid about specializing show for types ever since.

Copy link
Member

@ararslan ararslan left a comment

Choose a reason for hiding this comment

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

Always good to avoid what Jeff calls "type treason".

@c42f c42f merged commit 92c78e2 into master Apr 22, 2020
@c42f c42f deleted the cjf/remove-show-Type branch April 22, 2020 21:34
@c42f
Copy link
Member Author

c42f commented Apr 22, 2020

Thanks for chiming in here with the links! I only had a very vague memory of some of those past issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants