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

Fix issue #16976 - Do not convert blindly the size of an iterable. #6342

Closed
wants to merge 1 commit into from
Closed

Conversation

LemonBoy
Copy link
Contributor

Discuss, this might break some code relying on the implicit cast being done anyways.
Tests will follow suite once we come to an agreement.

@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
16976 Implicit conversion from ulong to int in foreach_reverse

{
if (!Type.tsize_t.implicitConvTo(fs.key.type))
{
fs.error("foreach: cannot implicitly convert the loop index from %s to %s",
Copy link
Contributor

Choose a reason for hiding this comment

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

If it can break valid code, it needs to be a deprecation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I did use error because the implicit casts failing throw an error, but if you feel this should follow the deprecation path first I'll change it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should never just break code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made it a deprecation instead and added a test

@LemonBoy
Copy link
Contributor Author

More discussion here, I'm inclined to make this a warning (or deprecation) for both the foreach and the foreach_reverse cases.

@UplinkCoder
Copy link
Member

D-runtime seems to rely on that behavior.
Also why does the test run on 32 bit ?
It seems to me like it should fail.

@Geod24
Copy link
Member

Geod24 commented Dec 25, 2016

Because sadly unsigned still implicitly converts to signed.

@UplinkCoder
Copy link
Member

The length parameter on 32bit is a uint not a ulong.
There the error message should not match.
Yet the auto-tester seemed to show green for 32bit targets.

@LemonBoy
Copy link
Contributor Author

LemonBoy commented Jan 4, 2017

If the VRP can determine that the index fits into the selected type no warning is issued.
Choosing a non-integral type for the loop index now throws an (hopefully clear) error.

Copy link
Member

@WalterBright WalterBright left a comment

Choose a reason for hiding this comment

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

needs rebase, though

@thewilsonator
Copy link
Contributor

see #8941

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.

8 participants