Opened 9 years ago

Last modified 5 years ago

#8766 new defect

document the _iadd_ and _imul_ special integer.pyx methods, which mutate self

Reported by: was Owned by: AlexGhitza
Priority: major Milestone: sage-6.4
Component: basic arithmetic Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by was)

I looked in integer.pyx at the two methods _iadd_ and _imul_, which both mutate self, e.g., allow for:

sage: a = 2010
sage: a._imul_(19)
sage: a
38190

I expected to find a bug exciting docstring about how these methods are unsafe, etc. Instead, there is *NOTHING* -- not even a doctest or docstring at all.

I also wish there were versions of these:

sage: a.unsafe_add_inplace
sage: a.unsafe_mul_inplace

that could be used in code, and would make their unsafe nature clear to the reader without having to read any docs.

Change History (7)

comment:1 Changed 9 years ago by was

  • Description modified (diff)

comment:2 Changed 9 years ago by was

> This is odd. From their names one would expect them to be used in __imul__
> and __iadd__ somewhere in the hierarchy, just like _repr_ is used in
> __repr__, so that they will be used for:
>
> sage: a = 1
> sage: a*=5
>
> as documented here: http://docs.python.org/reference/datamodel.html.
> However, this is not the case. It may be a bug (or yet to be implemented
> feature).

That is not a bug -- it is done on purpose.  The reason is because integers are meant to be *immutable*, since they have a hash method.   If _imul_ were used by __imul__, then people would be mutating integers left and right by accident, and vast amounts of code would consequently have subtle bugs all over the place.   

There might have been a time (maybe a few weeks in 2006) when __imul__ did indeed call _imul_ in Sage, so the name might be a historical remnant. 

Personally, I think the best thing would be:

  (1) Rename _imul_ and _iadd_ to something like _unsafe_inplace_mul_, _unsafe_inplace_add_

  (2) Document them. 

William

comment:3 Changed 9 years ago by was

This is odd. From their names one would expect them to be used in imul and iadd somewhere in the hierarchy, just like _repr_ is used in repr, so that they will be used for:

sage: a = 1 sage: a*=5

as documented here: http://docs.python.org/reference/datamodel.html. However, this is not the case. It may be a bug (or yet to be implemented feature).

That is not a bug -- it is done on purpose. The reason is because integers are meant to be *immutable*, since they have a hash method. If _imul_ were used by imul, then people would be mutating integers left and right by accident, and vast amounts of code would consequently have subtle bugs all over the place.

There might have been a time (maybe a few weeks in 2006) when imul did indeed call _imul_ in Sage, so the name might be a historical remnant.

Personally, I think the best thing would be:

(1) Rename _imul_ and _iadd_ to something like _unsafe_inplace_mul_, _unsafe_inplace_add_

(2) Document them.

William

comment:4 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:5 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:6 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:7 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4
Note: See TracTickets for help on using tickets.