-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
[SR-5964] dividedReportingOverflow may fail to compile for calculations that overflow #48523
Comments
I'm not sure what the desired behavior here is. There's no point in statically generating an overflow because you already know the answer, right? @moiseev, what do you think? |
The problem is that the reportingOverflow and regular functions are dispatch through the same function, and we do want to get the static overflow checks in case of regular ones. I'm on the fence about it, honestly. From where I stand, yes, it is a behavior that is inconsistent with the documentation in a subtle way, but it only behaves this way in rare cases where constant folding can figure out the concrete values, and as the description shows, it is even not the case for top-level... So anything non-trivial (as in, pretty much anything not-using literals) will compile just fine. Regardless, this is another manifestation of a known problem that we're going to address (see my first paragraph). Thanks for reporting it @martinr448! |
Related to: <rdar://problem/29937936> |
Comment by Peter W A Wood (JIRA) I tried Int8.min.dividedReportingOverflow(by: int8(-1)) in an iOS playground. It returns a tuple with partialValue of -128 and overflow of true. This is different from the behaviour in an Xcode playground. I don't know if the difference is due to the different playgrounds or the compiler. I first noticed this "edge case" issue when I was generating some tests to run in another language. I switched from Swift to C and found the same issue. Then I tried dividing the minimum integer value by -1 using C on both Intel and ARM processors. (The first in Xcode, the second using gcc on a Raspberry Pi). On Intel the division caused an overflow error but not on ARM. I thought it best to mention this in case it is relevant to the issue. |
/cc @stephentyrone on the above Intel vs ARM issue. |
Constant folding only gets smarter, so while this may effect only a few cases today, it will likely effect more cases in the future. We should be able to make this do what it says on the tin, and (I think) we all know that's ultimately the correct behavior. It sounds like there's two issues here–getting a static error and the Int8 behavior which is just giving the wrong answer entirely, right? |
Yes, the static error is clear, the overflow on different architectures is not, at least not to me =) |
Comment by Andrew Bennett (JIRA) +1 on this. The documentation states:
However there is an error " A satisfactory resolution, to me, would be to optimise out the call and replace it with If it helps find a satisfactory conclusion, I wanted to write a test to ensure that my custom |
It is not clear to me why “Division by zero” is a compile-time error at all. At most it should be a warning. After all, we don’t raise compile-time errors for other code which is guaranteed to trap at runtime, such as If the programmer writes code that always traps, they clearly want it to trap at runtime if execution reaches that point. |
Attachment: Download
Environment
macOS 10.12.6
Xcode 9.0 (9A235)
Apple Swift version 4.0 (swiftlang-900.0.65 clang-900.0.37)
Target: x86_64-apple-macosx10.9
Additional Detail from JIRA
md5: 3e39aeb0aec9075d1c78c3f6037e42b8
relates to:
Issue Description:
The documentation of
from the FixedWidthInteger protocol states that overflows are indicated by a flag in the return value, and that dividing by zero is not an error.
However, it may fail to even compile in cases where the compiler already detects an overflow or division by zero:
If the same code is on top-level then it compiles and runs as expected:
The text was updated successfully, but these errors were encountered: