Skip to content

fix: use 64 bit indices to avoid overflows#245

Merged
mhasself merged 1 commit into
masterfrom
index_overflow
Jul 7, 2026
Merged

fix: use 64 bit indices to avoid overflows#245
mhasself merged 1 commit into
masterfrom
index_overflow

Conversation

@skhrg

@skhrg skhrg commented Jul 3, 2026

Copy link
Copy Markdown
Member

Fixer #244

@skhrg skhrg requested review from mhasself and ykyohei July 3, 2026 20:36
@skhrg skhrg force-pushed the index_overflow branch from d1f83b5 to 8dbd227 Compare July 3, 2026 20:53
ykyohei
ykyohei reviewed Jul 3, 2026
View reviewed changes

@ykyohei ykyohei left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you for fixing this!

Comment thread src/array_ops.cxx Outdated
Comment thread src/array_ops.cxx Outdated
mhasself
mhasself previously approved these changes Jul 6, 2026
View reviewed changes

@mhasself mhasself left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't this the purpose of size_t? It's more self-documenting to use that. I won't insist now though...

@mhasself

mhasself commented Jul 6, 2026

Copy link
Copy Markdown
Member

Isn't this the purpose of size_t? It's more self-documenting to use that. I won't insist now though...

Whoops -- but please address @ykyohei 's question about the nblock calculations. If this change is truly needed, there should be a unit test to prove it.

@skhrg

skhrg commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

Isn't this the purpose of size_t? It's more self-documenting to use that. I won't insist now though...

I had a vague recollection that size_t was platform specific and once upon a time caused a bug for me when Mist was still around. But I guess we will probably never run on a Power CPU ever again...

@skhrg skhrg force-pushed the index_overflow branch from 8dbd227 to ae046e6 Compare July 7, 2026 15:15
@ykyohei ykyohei linked an issue Jul 7, 2026 that may be closed by this pull request
32-bit integer overflow of so3g.block_minmax #244
Open
@skhrg

skhrg commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

@mhasself @ykyohei any opposition to merging?

ykyohei
ykyohei approved these changes Jul 7, 2026
View reviewed changes

@ykyohei ykyohei left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No, looks good to me, thank you!

@skhrg

skhrg commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

Oh wait I don't have permission.. I guess I always had Matthew merge for me...

@mhasself mhasself merged commit e89f8b5 into master Jul 7, 2026
2 checks passed
@mhasself mhasself deleted the index_overflow branch July 7, 2026 17:39
@ykyohei

ykyohei commented Jul 8, 2026

Copy link
Copy Markdown

@mhasself Would it be possible to release a new version of so3g? I would really appreciate it if we could apply this fix on PyPI.

@mhasself

mhasself commented Jul 8, 2026

Copy link
Copy Markdown
Member

@mhasself Would it be possible to release a new version of so3g? I would really appreciate it if we could apply this fix on PyPI.

It's on its way ...

mhasself
mhasself reviewed Jul 10, 2026
View reviewed changes
Comment thread src/array_ops.cxx
auto max = std::max_element(data+start, data+stop);
val = *max - *min;
}
for(int si = start; si < stop; si++) {

@mhasself mhasself Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is the one you missed!

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

Reviewers

@mhasself mhasself mhasself left review comments
@ykyohei ykyohei ykyohei approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

32-bit integer overflow of so3g.block_minmax

3 participants

@skhrg @mhasself @ykyohei