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

Revert "Fixes #12187" #12447

Merged
merged 1 commit into from
Oct 18, 2019
Merged

Revert "Fixes #12187" #12447

merged 1 commit into from
Oct 18, 2019

Conversation

Araq
Copy link
Member

@Araq Araq commented Oct 17, 2019

Reverts #12321

This reverts commit 00c31e8.
@Clyybber
Copy link
Contributor

To be honest, I don't quite see the issue here. The packages have been fixed, this change has caused no real issues. The ecosystem has adapted to the breakage and now we revert this?
I think that will piss off a lot of pkg maintainers. (Also this change was a correction, not an opinionated change)

@Araq
Copy link
Member Author

Araq commented Oct 17, 2019

It does break wrappers not covered by "important packages" and also the change should have been something like:

type 
  csize {.deprecated: "use csize_t instead".}
  csize_t = <correct definition here>

Also, the fact that you had to add overloads that do the same "lie" about signedness means that the old csize's definition actually was better.

@Clyybber
Copy link
Contributor

Well those overloads were added deprecated, meaning that it was planned to remove them after some time.

@krux02
Copy link
Contributor

krux02 commented Oct 17, 2019

I think it is a bad idea to lie to the developer that csize is what size_t is in c++. It think it can be dangerous if signedness of a type is not what the developer thiks what it is. If you really really really want to reintroduce this hack, pleas explain why you think it won't be a problem.

@krux02
Copy link
Contributor

krux02 commented Oct 17, 2019

Here, this compiles with your revert applied

proc main() =
  var x: csize = -1
  var y: csize = 123
  echo x, " < ", y, ": ", x < y

main()

output:

-1 < 123: false

Please don't tell me this is acceptable expected behavior.

@Araq
Copy link
Member Author

Araq commented Oct 17, 2019

3 replies, all managed to miss the point entirely. Good job.

It does break wrappers not covered by "important packages" and also the change should have been something like:

type 
  csize {.deprecated: "use csize_t instead".}
  csize_t = <correct definition here>

@krux02
Copy link
Contributor

krux02 commented Oct 18, 2019

3 replies, all managed to miss the point entirely. Good job.

I guess we are all stupid then. It is all our fault.

It does break wrappers not covered by "important packages" and also the change should have been something like: [...]

That is not what this PR does either.

@bluenote10
Copy link
Contributor

3 replies, all managed to miss the point entirely.

An explanation would really help. I don't see why that is a step forward either. So far the argument for keeping this bug (to me that's a bug) is that running into problems has a low probability, but doesn't that make it even more ugly?

@Araq Araq merged commit 889b745 into devel Oct 18, 2019
@Araq Araq deleted the revert-12321-csize_uint_tests branch October 18, 2019 13:59
@Araq
Copy link
Member Author

Araq commented Oct 18, 2019

Reverted because it broke code. I dunno what's so hard to understand about it.

@FedericoCeratto
Copy link
Member

I support a smoother deprecation process, like the one Araq suggested, because the abrupt change in #12321 creates conditions where csize has different meaning in Nim 1.0.0 and the next release, making it difficult to maintain backward compatibility in libraries.
Also, the deprecation process provides a friendly warning to developers instead of introducing a sudden breaking change.

@krux02
Copy link
Contributor

krux02 commented Oct 23, 2019

@FedericoCeratto

I support a smoother deprecation process, like the one Araq suggested, because the abrupt change in #12321 creates conditions where csize has different meaning in Nim 1.0.0 and the next release, making it difficult to maintain backward compatibility in libraries.

Well it is true that this is an abrupt change. But it isn't hard to maintain backwards compatibility with this change. Explicit conversions like csize(x) or cast[csize](x) will work no matter if csize in defined as int or uint. A new type csize_t and deprecating csize would force you to either fix the deprecation warning, drop older nim versions, or copy the definition of csize_t locally into the project to make it compatible with old versions of Nim.

alehander92 pushed a commit to alehander92/Nim that referenced this pull request Dec 2, 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.

5 participants