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

[DOC] clarify safety of integer utils #17539

Closed
wence- opened this issue Dec 6, 2024 · 2 comments · Fixed by #17542
Closed

[DOC] clarify safety of integer utils #17539

wence- opened this issue Dec 6, 2024 · 2 comments · Fixed by #17542
Assignees
Labels
bug Something isn't working

Comments

@wence-
Copy link
Contributor

wence- commented Dec 6, 2024

Describe the bug

Noticed while implementing saturating addition for rolling window work.

This is a minor thing, and I think all the usages in libcudf are safe anyway, but...

In integer_utils.hpp we have (for signed integers).

template <typename I>
constexpr I div_rounding_up_safe(std::integral_constant<bool, true>, I dividend, I divisor) noexcept
{
  auto quotient  = dividend / divisor;
  auto remainder = dividend % divisor;
  return quotient + (remainder != 0);
}

This doesn't work for divisor == 0 (OK, fine). It also overflows for divisor == -1 since INT_MIN / -1 is undefined.

I think all the usages in libcudf are for dividing two positive integers, in which case it's probably fine.

@wence- wence- added the bug Something isn't working label Dec 6, 2024
@davidwendt
Copy link
Contributor

This is an internal function so there is no binding contract here and 'safe' is a relative term.
Do you have a problem with the name? I don't think it is worth renaming perhaps.
Do you wish for a safer version that checks the conditions you mentioned?
How would you like to see this issue closed?

@wence-
Copy link
Contributor Author

wence- commented Dec 6, 2024

How would you like to see this issue closed?

Probably just to document these edge cases.

@wence- wence- self-assigned this Dec 6, 2024
@wence- wence- changed the title [BUG] "safe" integer utils are less safe than advertised [DOC] clarify safety of integer utils Dec 6, 2024
@rapids-bot rapids-bot bot closed this as completed in ed2892c Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants