Skip to content
This repository has been archived by the owner on Mar 23, 2023. It is now read-only.

Add support for the abs builtin #28

Merged
merged 1 commit into from
Jan 6, 2017
Merged

Add support for the abs builtin #28

merged 1 commit into from
Jan 6, 2017

Conversation

meadori
Copy link
Contributor

@meadori meadori commented Jan 5, 2017

Implement the abs as defined here:

Copy link
Contributor

@trotterdylan trotterdylan left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together! Please just confirm that the command "make precommit" succeeds.

@meadori
Copy link
Contributor Author

meadori commented Jan 5, 2017

make precommit passed.

@trotterdylan
Copy link
Contributor

trotterdylan commented Jan 5, 2017 via email

@meadori
Copy link
Contributor Author

meadori commented Jan 5, 2017

Thanks for catching that! I reworked the patch to use the __abs__ slot instead.

Copy link
Contributor

@trotterdylan trotterdylan left a comment

Choose a reason for hiding this comment

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

Couple more small suggestions. Also a friendly reminder to make precommit (I will get Travis doing that soon...)

func intAbs(f *Frame, o *Object) (*Object, *BaseException) {
z := toIntUnsafe(o).Value()
if z <= 0 {
z = -z
Copy link
Contributor

Choose a reason for hiding this comment

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

This can overflow if z == MinInt so check for that case and promote to Long.

@@ -66,6 +66,14 @@ func (i *Int) IsTrue() bool {
// IntType is the object representing the Python 'int' type.
var IntType = newBasisType("int", reflect.TypeOf(Int{}), toIntUnsafe, ObjectType)

func intAbs(f *Frame, o *Object) (*Object, *BaseException) {
z := toIntUnsafe(o).Value()
if z <= 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid an unnecessary allocation in the positive case, how about:

z := toIntUnsafe(o)
if z.Value() > 0 {
  return z.ToObject(), nil
}
// .. check for overflow as mentioned below
return NewInt(-x).ToObject(), nil

@meadori
Copy link
Contributor Author

meadori commented Jan 6, 2017

Thanks for the review. I address the copy for > 0 and overflow issues.

Copy link
Contributor

@trotterdylan trotterdylan left a comment

Choose a reason for hiding this comment

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

This is great! Thanks for getting this done.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants