-
Notifications
You must be signed in to change notification settings - Fork 987
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
Add 1+, 1- and -1+, and improve abs
in cptypes
#888
Conversation
Thank you for the updates! Please add a subsection to the release notes describing them. |
9750059
to
6bab17a
Compare
I fixed one misplace parenthesis that was causing an error in one of the tests. I added a note for each commit. It is it fine or it's better to accumulate all the changes in a single note? But I did't add this text in the notes to avoid repetition. I can add it if you think it's necessary.
|
release_notes/release_notes.stex
Outdated
\subsection{Improved support abs in type recovery (10.2.0)} | ||
|
||
The type recovery pass has improved support for abs when the argument is a fixnum. | ||
|
||
\subsection{Add support for 1+, 1- and -1+ in type recovery (10.2.0)} | ||
|
||
The type recovery pass has support for 1+, 1- and -1+. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd combine these into a single entry, something like:
\subsection{Type recovery improvements (10.2.0)}
The type recovery pass has improved support for \scheme{abs} with a fixnum argument
and added support for \scheme{1+}, \scheme{1-}, and \scheme{-1+}.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine not to add the sentence about enabling type recovery.
6bab17a
to
f53a364
Compare
Done |
f53a364
to
47c4411
Compare
'(lambda (x) (when (bignum? x) | ||
(bignum? (sub1 x)))) | ||
'(lambda (x) (when (bignum? x) | ||
#t)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this test have been kept? I don't think it's covered by the test-closed1
generalization, but I might be missing something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I undeleted the test. It's a to ensure I don't make an easy mistake in the future. I don't know why I deleted it.
It's not included in test-closed1
. Moreover, I was thinking about writing test-not-closed1
to reduce duplication, but I decided that it was too weird. But Perhaps I'll add it in the future if there are too many cases of almost-closed functions.
This extends the reductions for add1 and sub1 to the other variants.
Add a special case for `abs`, in particular because (abs (most-negative-fixnum)) is not a fixnum.
47c4411
to
cbcd12b
Compare
Two slightly relate patchs.
Add 1+, 1- and -1+ to cptypes
I should have added them in the (old) previous commit that added
add1
andsub1
, but I forget these primitives existed. I refactored some of the test to avoid too much duplication.Improve fixnum case of abs in cptypes
In the (old) previous commit I added a reduction for
(abs <flnumber>)
to(flabs <flnumber>)
. But it has no reduction for(abs <fxnumber>)
.Now I'm reducing
(abs <fx>)
tothat is the straightforward expansion. It's expanded later in
cpprim
toBut perhaps it's better to expand it in
cptypes
directly tobecuse it is faster for positive numbers and slower for
(most-negative-fixnum)
that is very unusual.I also wanted to add a similar special case for
add1
andsub1
, but in their expansion incpprim
they use theoverflow
flag, so expanding them to a comparison to(most-negative-fixnum)
or(most-positive-fixnum)
is probably slower. (I hope to try something like this next month.)