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

deprecate WString and wstring #16975

Merged
merged 1 commit into from
Jun 18, 2016
Merged

deprecate WString and wstring #16975

merged 1 commit into from
Jun 18, 2016

Conversation

StefanKarpinski
Copy link
Sponsor Member

No description provided.

if sizeof(Cwchar_t) == 2
@deprecate_binding WString UTF16String
@deprecate_binding wstring utf16
utf16(s::Cwstring) = utf16(convert(Ptr{Cwchar_t}, s))
Copy link
Contributor

@tkelman tkelman Jun 16, 2016

Choose a reason for hiding this comment

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

tab again

more consequentially, these deprecations are platform-dependent which defeats the purpose of them having a separate name

Copy link
Sponsor Member Author

@StefanKarpinski StefanKarpinski Jun 16, 2016

Choose a reason for hiding this comment

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

I'm not sure how to fix that since this was an alias anyway ¯\_(ツ)_/¯

Copy link
Contributor

@tkelman tkelman Jun 16, 2016

Choose a reason for hiding this comment

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

we should probably recommend something that involves if is_windows() a; else b; end

(or equivalent with sizeof(Cwchar_t) if that's not going to be deprecated)

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Right, but the binding deprecation isn't going to be possible to handle that way anyhow.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I'm going to leave this since the next step is to delete UTF16 and UTF32 anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't see this since it was collapsed, but what's the replacement going to be here? Wherever that moves to, this deprecation should direct to something correct.

@StefanKarpinski StefanKarpinski force-pushed the sk/rm-wstring branch 3 times, most recently from 2c7372f to cb23547 Compare June 17, 2016 21:22
@tkelman
Copy link
Contributor

tkelman commented Jun 18, 2016

appveyor failures are real here, just so your merge finger doesn't get too itchy

@StefanKarpinski
Copy link
Sponsor Member Author

YESSS! I finally got this bugger to pass on Windows.

@StefanKarpinski
Copy link
Sponsor Member Author

Failure is a timeout. The other Windows run passed. Tempted to merge...

@StefanKarpinski StefanKarpinski merged commit fe7f36c into master Jun 18, 2016
@StefanKarpinski StefanKarpinski deleted the sk/rm-wstring branch June 18, 2016 22:43
@tkelman
Copy link
Contributor

tkelman commented Jun 19, 2016

The deprecation here is pretty flawed and is going to lead people to write code that doesn't behave cross platform the way they thought it might from using these aliases.

@tkelman
Copy link
Contributor

tkelman commented Jun 19, 2016

Compat also needs fixing for this.

@StefanKarpinski
Copy link
Sponsor Member Author

Once we delete UTF(16|32)String, these deprecations have to be deleted. I'm not sure what they can be replaced with unless we keep the entirety of those types around in base/deprecated.jl, which seems insane.

@tkelman
Copy link
Contributor

tkelman commented Jun 19, 2016

It should redirect to something that does the right thing, whether or not that's in base. If that doesn't exist yet, then we're not ready to remove the types.

@StefanKarpinski StefanKarpinski mentioned this pull request Jul 5, 2016
32 tasks
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.

2 participants