Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#30601 closed enhancement (fixed)

Move bitset.pxi to bitset_base.pxd

Reported by: gh-kliem Owned by:
Priority: major Milestone: sage-9.2
Component: refactoring Keywords: bitset, header
Cc: ​vdelecroix Merged in:
Authors: Jonathan Kliem Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: bc0e1fb (Commits, GitHub, GitLab) Commit: bc0e1fb713d8301f93bf4f86244b3a65498f891a
Dependencies: #30597 Stopgaps:

Status badges

Description

We move bitset.pxi to bitset_base.pxd.

So far bitset.pxi contained mostly inline functions for the header but also a few functions that cannot be inlined due to optional arguments.

https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html?highlight=pxi#cython-file-types

Change History (8)

comment:1 Changed 2 years ago by gh-kliem

Status: newneeds_review

comment:2 Changed 2 years ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw

Modern Cython uses range, so

-    for i from 0 <= i < bits.size:
+    for i in range(bits.size):
         s[i] = one if bitset_in(bits, i) else zero

If you could make these changes where appropriate, that would be good. Beyond that, LGTM.

comment:3 Changed 2 years ago by git

Commit: d5e06fdbb680b516551a782d7a81db51618b0ad0bc0e1fb713d8301f93bf4f86244b3a65498f891a

Branch pushed to git repo; I updated commit sha1. New commits:

bc0e1fbremove old style loop instructions

comment:4 in reply to:  2 ; Changed 2 years ago by gh-kliem

Done.

I figured it was some old style usage.

Replying to tscrim:

Modern Cython uses range, so

-    for i from 0 <= i < bits.size:
+    for i in range(bits.size):
         s[i] = one if bitset_in(bits, i) else zero

If you could make these changes where appropriate, that would be good. Beyond that, LGTM.

comment:5 in reply to:  4 Changed 2 years ago by Travis Scrimshaw

Status: needs_reviewpositive_review

Replying to gh-kliem:

Done.

Thank you.

I figured it was some old style usage.

You're probably right, but it was something I noticed from the diff and I didn't look too closely if it was a hold-over or an addition. (Although I do try and change these things when I do such changes, but that is me.)

comment:6 Changed 2 years ago by gh-kliem

Thank you.

I think range code is much more readable for a person coming from python, so I will change, when I stumble upon it.

comment:7 Changed 2 years ago by Volker Braun

Branch: u/gh-kliem/properly_define_bitsets_with_a_headerbc0e1fb713d8301f93bf4f86244b3a65498f891a
Resolution: fixed
Status: positive_reviewclosed

comment:8 Changed 2 years ago by Matthias Köppe

Milestone: sage-9.3sage-9.2
Note: See TracTickets for help on using tickets.