Skip to content

Guard against overflow for dynamic interval overlap - #60

Merged
5cript merged 11 commits into
5cript:masterfrom
whimxiqal:bug/overlap-overflow
Jun 25, 2026
Merged

Guard against overflow for dynamic interval overlap#60
5cript merged 11 commits into
5cript:masterfrom
whimxiqal:bug/overlap-overflow

Conversation

@whimxiqal

@whimxiqalwhimxiqal commented May 13, 2026•
edited
Loading

Copy link
Copy Markdown
Contributor

The dynamic interval overlaps method contains addition code to the boundaries regardless of whether those values would overflow. This can cause wrong results at, at the very least, compiler errors depending on the compiler options.

These changes guard against such addition/subtraction and add tests with additional compiler options to error on integer overflow.

I've also adjusted the workflow, CMake, and README files to streamline testing a bit.

@whimxiqal
whimxiqal marked this pull request as ready for review June 24, 2026 19:12
@whimxiqalwhimxiqal changed the title Overlap check results in integer overflow with maximum integer boundaryGuard against overflow for dynamic interval overlapJun 24, 2026
@whimxiqal

whimxiqal commented Jun 24, 2026•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Fixes #61

@5cript

5cript commented Jun 25, 2026

Copy link
Copy Markdown
Owner

Thank you for the contribution
I will review this as soon as I am able.

@5cript5cript self-assigned this Jun 25, 2026
5cript
5cript reviewed Jun 25, 2026
View reviewed changes
Comment threadtests/CMakeLists.txt Outdated
@5cript

5cript commented Jun 25, 2026•
edited
Loading

Copy link
Copy Markdown
Owner

I did some commits on top:
whimxiqal:bug/overlap-overflow...5cript:bug/overlap-overflow-review

Can i turn this review upside down? :D
I think the fix you introduced doesnt work. Because it disregards the negative numbers.
I added some tests to cover for that and I think fixed the problem properly.

I can put them on top of your contribution or follow them up afterwards. Or you cherry pick them yourself
Waiting for reply

@whimxiqal

whimxiqal commented Jun 25, 2026

Copy link
Copy Markdown
ContributorAuthor

Ah good point with the negative numbers. Cherry-picked!

@whimxiqal
whimxiqal requested a review from 5criptJune 25, 2026 18:23
5cript
5cript approved these changes Jun 25, 2026
View reviewed changes
Comment threadREADME.md
```bash
mkdir build
cd build
cmake --build .

@5cript5criptJun 25, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

cmake .. -DINT_TREE_ENABLE_TESTS=on

Configure step is missing.

Comment threadtests/CMakeLists.txt

find_package(GTest REQUIRED)
include(FetchContent)
FetchContent_Declare(

@5cript5criptJun 25, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Yes this is easier and I only accept this because its for tests only where it doesnt matter.
FetchContent cannot be used in network isolated contexts such as flatpak and yocto.

Just letting you know.

@5cript
5cript merged commit 133919e into 5cript:masterJun 25, 2026
@5cript

5cript commented Jun 25, 2026

Copy link
Copy Markdown
Owner

Thanks a lot for improving this library!

@5cript5cript mentioned this pull request Jun 25, 2026
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@5cript5cript5cript approved these changes

Assignees

@5cript5cript

Labels

bugenhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@whimxiqal@5cript