Skip to content
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

Move bitset.pxi to bitset_base.pxd #30601

Closed
kliem opened this issue Sep 18, 2020 · 8 comments
Closed

Move bitset.pxi to bitset_base.pxd #30601

kliem opened this issue Sep 18, 2020 · 8 comments

Comments

@kliem
Copy link
Contributor

kliem commented Sep 18, 2020

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

Depends on #30597

CC: @videlec

Component: refactoring

Keywords: bitset, header

Author: Jonathan Kliem

Branch/Commit: bc0e1fb

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/30601

@kliem kliem added this to the sage-9.3 milestone Sep 18, 2020
@tscrim
Copy link
Collaborator

tscrim commented Sep 19, 2020

comment:2

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.

@tscrim
Copy link
Collaborator

tscrim commented Sep 19, 2020

Reviewer: Travis Scrimshaw

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 21, 2020

Changed commit from d5e06fd to bc0e1fb

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 21, 2020

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

bc0e1fbremove old style loop instructions

@kliem
Copy link
Contributor Author

kliem commented Sep 21, 2020

comment:4

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.

@tscrim
Copy link
Collaborator

tscrim commented Sep 21, 2020

comment:5

Replying to @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.)

@kliem
Copy link
Contributor Author

kliem commented Sep 21, 2020

comment:6

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.

@vbraun
Copy link
Member

vbraun commented Sep 30, 2020

Changed branch from u/gh-kliem/properly_define_bitsets_with_a_header to bc0e1fb

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

No branches or pull requests

4 participants