Skip to content

Conversation

@ocstl
Copy link
Contributor

@ocstl ocstl commented Nov 7, 2019

Version 1.2.0 modifies the input values so that the number of seconds fits into an i32. The expected values are updated to reflect the change.

Relevant PRs:

Version 1.2.0 modifies the input values so that the number of seconds
fits into an `i32`. The expected values are updated to reflect the
change.

Relevant PRs:
 - 1.1.0 -> 1.2.0: exercism/problem-specifications#1441
Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

Here is a question. I Approve either way. If no answer, 72 hours for other maintainers to object.

Since they tell us the numbers will now fit into i32, plus our knowledge that there are no negatives, we could change our stub from From<u64> to From<u32>. But can/should we?

@coriolinus
Copy link
Member

We could go as far as to change the stub to From<i32>. I believe the intent of the 1.2.0 change was to enable exactly that conversion. Should we, though?

A brief googling informs me that in the pre-1.0 Rust community, there were some community members who felt strongly that the "default" integer type should be i32. As far as I can tell, the reasoning was something like "it's reasonably efficient, is big enough in most cases, and is in line with what a C programmer would expect from an int." Post-1.0, I haven't seen a ton of discussion about it.

I'm reluctant to change this exercise's default from u64 to i32. My rationale for this reluctance is simply that it's not causing any problems, and diffs should exist for a particular reason, not a general-purpose "this is possible, so why not".

@ocstl
Copy link
Contributor Author

ocstl commented Nov 7, 2019

Thank you both for the reviews!

I'm not strongly opposed to changing it to either u32 or i32, but I agree with @coriolinus that, since it's not causing any issues, it's probably better to leave it as is, especially since all current solutions are built on that constraint.

While it is true that an unconstrained literal defaults to i32, there's no "basic" integer type per se, Rust being more explicit, contrary to the int in other languages, which was the motivation for the change (to avoid introducing new types too early).

And, even there, it's geared towards specific languages (C# for instance) where an int is 32 bits in size; C's int being at least 16 bits, the original example had to use a long, but even that raised issues for implementations of a long as 32 bits (exercism/c#382). Which probably proves some point about the benefits of explicitness. 😀

I see mostly issues with changing it, but I'm still open about changing my mind on this though.

@petertseng petertseng merged commit 06234db into exercism:master Nov 9, 2019
@ocstl ocstl deleted the doc-space-age branch November 10, 2019 02:20
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.

3 participants