Opened 6 years ago

Closed 5 years ago

#15987 closed enhancement (fixed)

Python 3 preparation: Change syntax of octal integer literals

Reported by: wluebbe Owned by:
Priority: major Milestone: sage-6.4
Component: distribution Keywords: python3
Cc: Merged in:
Authors: Wilfried Luebbe Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: a17cb52 (Commits) Commit: a17cb523eba3d523e71d3a2c4f9b4bf90ad44ef7
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

The deprecated syntax is not accepted by Python 3.

Changes according to lib2to3/fixes/fix_numliterals.py:

0755 into 0o755

This ticket is tracked as a dependency of meta-ticket ticket:16052.

Attachments (2)

long-literal.warn (3.0 KB) - added by wluebbe 6 years ago.
search result for integer literals with suffix "L" (type long)
long-literal.warn-details-both (14.5 KB) - added by wluebbe 6 years ago.
More complete search result for long integer literals

Download all attachments as: .zip

Change History (19)

Changed 6 years ago by wluebbe

search result for integer literals with suffix "L" (type long)

comment:1 follow-up: Changed 6 years ago by wluebbe

  • Branch set to u/wluebbe/ticket/15987
  • Commit set to a17cb523eba3d523e71d3a2c4f9b4bf90ad44ef7
  • Status changed from new to needs_review

The changes for the new octal literal format should be complete: 2 modules were fixed by the 2to3 tool and another case was manually fixed in a doc-test.

2to3 has found nothing for the long integer literal (suffix "L").
13 modules were found with regexp search (r"\s (0\d+) [\s)]"). The result file is attached.

  • 6 pyx modules from sage/rings/padics contain code like (1L << (sizeof(long) * 8 - 2)) - 1. I do not whether these should be changed.
  • 4 modules had doc-test with stuff like 4561L # 32-bit. Perhaps these test can be made more tolerant.
  • The module sage/interfaces/r.py had strings like
    "rel_re_integer.sub(a._subs_integer, ' 1L 2L')". Maybe this could / should be changed.
  • 2 modules in sage/combinat had the text "ValueError?: R10L is not a correct map". Nothing to change.

New commits:

a17cb52changed to new octal literal format (in 3 modules)

comment:2 Changed 6 years ago by wluebbe

I did

git merge u/chapoton/15991 u/wluebbe/ticket/15986 u/wluebbe/ticket/15987

on top of u/wluebbe/ticket/15992 (which is rebased on 6.2.beta5)

Test report:

./sage -t -p --all --long --logfile=logs/ptestlong-alle4.log
...
All tests passed!

comment:3 in reply to: ↑ 1 Changed 6 years ago by ohanar

Replying to wluebbe:

2to3 has found nothing for the long integer literal (suffix "L").
13 modules were found with regexp search (r"\s (0\d+) [\s)]"). The result file is attached.

  • 6 pyx modules from sage/rings/padics contain code like (1L << (sizeof(long) * 8 - 2)) - 1. I do not whether these should be changed.

These should be fine, long integer literals are important to C and hence you cannot always drop them in Cython code (even for Cython written specifically written for Python 3).

  • 4 modules had doc-test with stuff like 4561L # 32-bit. Perhaps these test can be made more tolerant.

How do you propose? The issue is that the Cython code is likely written with 64-bit integers, which on 32-bit platforms cannot always be placed into a Python2 int (since they are just a simple wrapper for the underlying platforms C long). In general you will get this issue when dealing with large integers, it is just mostly avoid in sage since we usually return Sage integers instead of Python ints or longs.

comment:4 follow-up: Changed 6 years ago by wluebbe

I was thinking about #doctest: +ELLIPSIS and replacing 4561L # 32-bit by 4561... # 32-bit.

I hope this would not run counter the purpose of the test :-)

comment:5 in reply to: ↑ 4 Changed 6 years ago by ohanar

Replying to wluebbe:

I was thinking about #doctest: +ELLIPSIS and replacing 4561L # 32-bit by 4561... # 32-bit.

I think ... is too liberal, e.g. the follow function would pass this doctest:

def foo():
    print("4561"+"lots of other text"*1000)

comment:6 Changed 6 years ago by wluebbe

How about adding some magic number as a sentinel after 4561L?

E.g change

            sage: mm[1]
            10009             # 64-bit
            10009L            # 32-bit

into

            sage: mm[1], 314159
            (10009, 314159)             # 64-bit
            (10009L, 314159)            # 32-bit

comment:7 Changed 6 years ago by wluebbe

Of course I meant (10009..., 314159) # 32-bit :-/

comment:8 Changed 6 years ago by ohanar

I would rather just wrapping the offending doctests with Integer and providing a comment on why the seemingly unnecessary Integer wrap is there.

comment:9 Changed 6 years ago by wluebbe

  • Status changed from needs_review to needs_work

Changed 6 years ago by wluebbe

More complete search result for long integer literals

comment:10 Changed 6 years ago by wluebbe

The first search was incomplete. A lot more \d+L instances found (see new attachment).

I guess "fix_long" ticket:16072 (stage 2) should be solved first. That will clarify the approach to be selected here.

Move to stage 2?

comment:11 Changed 6 years ago by wluebbe

  • Description modified (diff)

comment:12 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:13 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:14 Changed 5 years ago by jdemeyer

  • Description modified (diff)
  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_work to positive_review
  • Summary changed from Python 3 preparation: Change syntax of long and octal integer literals to Python 3 preparation: Change syntax of octal integer literals

comment:15 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

author name

comment:16 Changed 5 years ago by jdemeyer

  • Authors set to Wilfried Luebbe
  • Status changed from needs_work to positive_review

comment:17 Changed 5 years ago by vbraun

  • Branch changed from u/wluebbe/ticket/15987 to a17cb523eba3d523e71d3a2c4f9b4bf90ad44ef7
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.