-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[WIP] Arithmetic with Overflow #6223
Conversation
since it's done in codegen_primitive and others codegen_binary avoids setting it
More linear code for operators
for exact location information of the exception in the caller context
Make default unchecked to keep current semantics but schedule change on the default policy
Move OverflowCheck to OverflowCheckScope::Policy
intrinsics are added on demand
Changing overflow_check should invalidate cache
Removing any of these prevent grisu3_specs to pass
Int#** avoid k *= k in last iteration due to possible overflow
Otherwise generated enum values might overflow (even if they are not used due to manual assignment)
@wontruefree The only symbols that are left besides Unless your keyword layout has something I am not seeing, or we go to with heavy signs ➕➖✖➗(they are not easy to type!), the option to go seems to be |
When I first thought about these operators, I thought about |
(that said, I'm fine with |
I'm fine with BTW: I found it confusing that swift names overflowing operators to the one that after an overflow will wrap around |
I'd also assume that overflowing in this case means that's allowed to overflow. |
Following up on what @asterite said, it could be a char after the operator: So like: |
thank @bcardiff that makes a lot of sense. |
@bew But the standard operators But then, |
Oh right thanks @straight-shoota, I've updated my message accordingly. |
Yeah, let's avoid something where a wrong space is still valid syntax with a different meaning even if it then would fail the semantic phase, as in |
Maybe |
If use suffix instead of prefix, then why not same It could be read as "add with and", "sub with and" etc, that is close to logical semantic:
Though, prefix looks a bit prettier. |
|
I think |
I.e. no need to bother to be smart or clever. Swift paved the way. Let's follow. |
Sorry to add more confusion. Is it possible to use "wrapping" characters?
e.g. [+] |+| <+> _+_ . They are longer but we already have <=> and ===
. (Ignore me if this does not make any sense to you :D)
…On Mon, Jul 2, 2018 at 6:21 PM Julien Portalier ***@***.***> wrote:
I.e. no need to bother to be smart or clever. Swift paved the way. Let's
follow.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#6223 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAz4ZGlgyTpoMOGFetsz5AGF15N1HL8lks5uCo7YgaJpZM4Ut7us>
.
--
-----------------------------------------------------
.^. In an open world, who needs windows or gates?
/V\ Cristian Molina <http://www.megatux.me/>
// \\ GNU/Linux User #73047, Ubuntu User # 14733
/( _ )\ San Luis - Argentina
^^ ^^ ---------------------------------------------
|
@megatux: I think the road to malbolge is paved with something like that :) |
I am curious what is the progress on this? |
The amp ops will have the right semantics in 0.27. I will extract those changes from old branch probably next week. I’m glad there was interest in the numerical features in the last days! |
I just saw the amp ops PR go threw! |
@wontruefree no, 0.26 had the operators but no implementation. 0.27 will have |
This PR adds overflow to integer arithmetics, adds new language constructs to allow user
to choose where overflowing (
checked
) or wrapping (unchecked
) semantic is desired in a given lexical scope.It also updates the STD where
unchecked
semantic is required. Or at least, where specs began to fail if run with--overflow-checked
.Updates in the compiler options
There are two compiler options
--overflow-checked
and--overflow-unchecked
to change thedefault overflow policy to be used. Currently the default is
--overflow-unchecked
to keep the currentsemantic and allow code to be updated more easily. In future versions I think the default should be
--overflow-checked
in favor of safety.After building a crystal compiler you build with
--overflow-checked
option and run specs as follows.Updates in the language
The operations affected by the overflow policy are only
+
,-
,*
between integers.The new language constructs are:
The type and value of the whole
[un]checked { <expr> }
matches the type and value of<expr>
or it raise anOverflowError
.When an
OverflowError
is raised the location advertised by the exception is/should be the location of the operator that generate the overflow.Lexical Scope
The intention of using a lexical scope is that it would make as easy as possible to know how the operation is performed.
When compiled with
--overflow-checked
,Regarding blocks the idea is the same, when compiled with
--overflow-unchecked
,Implementation details
Given that
+
,-
,*
forInt
are primitives they are always inlined and the codegen stage knows what to do along the way.It is easy to change the semantic of the operation give some context, in this case the lexical scope information.
The current behaviour and under the
unchecked
policy the+
is translated toadd
IR.Under the
checked
policy the+
is translated tollvm.sadd.with.overflow.*
a LLVM with Overflow Intrinsics.Note: In both situations when the operands do not belong to the same type some expansion, truncation and additional checks for overflow are done. This is because the operation is carried on the "bigger" type, and the result is always translated to the type of the left operand. This is the current semantic.
Limitations of the scopes
While developing the feature I came across two operations that would be nice to overflow along with
+
,-
,*
.Int#**(exponent : Int)
Int#-()
This operation are not primitives as the others, but they are operators. I think it would be nicer if their overflow semantic is changed together with the current overflow scope policy.
Maybe for now,
Int#**(exponent : Int)
got its counterpartInt#unchecked_pow(exponent : Int)
since the former will match the compiler policy that will eventually be checked.The second one was used in one single place
as
-info.value.to_{{method}}
and changed tounchecked { 0.to_{{method}} - info.value.to_{{method}} }
.These two cases are methods and they are not inlined, so the mechanism described in "Implementation details" does not longer apply.
If we want them to be affected by the overflow policy the possible paths to go are:
@[AlwaysInline]
and make the inline methods be affected by the policy.checked
or theunchecked
version directly.Inlining the
**
might not be that nice since it's ~10 lines and used it in many places. Adding the annotation seems a bit to much for a first round.Another usage of forwarding the policy to other methods I can imagine is for example in matrix operations: If
*
is to be applied between matrices or matrix-scalar the user might have fine-grained control beyond the global policy (--overflow-[un]checked
). But I found that either the packages always wrap silently or use floats.So, all in all, up to here the only real candidate was
**
, hence theunchecked_pow
method.Standard Library changes
As a migration technique a temporal macro
__next_unchecked
is added and should be replaced withunchecked
on the release after merge.These changes are marked as
Random: Mark required unchecked code blocks
or different std-lib sections. All of these change can be changed later specially Time and JSON. They are focused of keeping the current behavior mostly. In the case of Time, there was some manual overflow check.All of these changes were pushed due to existing specs that failed otherwise.
Pending
[un]checked do; end
notation also. I like that{ }
fits better to be used inside expressions.256.to_u8
should raise in checked context)[un]checked
scopeOverflowCheckScope
ast node)Feedback is welcome