clyne/constexpr-to-string
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Potential problem with negative minimum #2

New Issue
Closed
opened 2020-06-26 15:48:27 -04:00 by EnilPajic · 2 comments
EnilPajic commented 2020-06-26 15:48:27 -04:00 (Migrated from github.com)
Copy Link

Hello. First of all, good work here!

The code N < 0 ? -N : N could be problematic in some cases if N is minimum value. E.g if signed integrals are defined as two's complement, then for signed char range may be -128 to 127, and problem with -N is when N = -128 (+128 may be not valid for signed char).

I've created PR to fix this.

Hello. First of all, good work here! The code `N < 0 ? -N : N` could be problematic in some cases if `N` is minimum value. E.g if signed integrals are defined as two's complement, then for `signed char` range may be -128 to 127, and problem with `-N` is when `N = -128` (+128 may be not valid for `signed char`). I've created [PR](https://github.com/tcsullivan/constexpr-to-string/pull/1) to fix this.
tcsullivan commented 2020-06-26 17:40:52 -04:00 (Migrated from github.com)
Copy Link

Hi, thanks for reporting this issue! I've checked out your proposed fix, and while it is addressing this issue, it appears to be breaking the conversion of other negative numbers. I'm starting to check out fixes for this issue too; do let me know if you make more progress.

Hi, thanks for reporting this issue! I've checked out your proposed fix, and while it is addressing this issue, it appears to be breaking the conversion of other negative numbers. I'm starting to check out fixes for this issue too; do let me know if you make more progress.
tcsullivan commented 2020-06-27 19:06:47 -04:00 (Migrated from github.com)
Copy Link

I've found the issue: looks like the base template parameter being unsigned int breaks the negative arithmetic. Can fix that just by switching base to int.

I'm new to working with pull requests/issues, but I'm going to try and make that edit to your PR. If that works, I'll merge it in and resolve this issue.

Edit: Was able to make the edit and merge the PR. All looks good!

I've found the issue: looks like the `base` template parameter being `unsigned int` breaks the negative arithmetic. Can fix that just by switching `base` to `int`. I'm new to working with pull requests/issues, but I'm going to try and make that edit to your PR. If that works, I'll merge it in and resolve this issue. **Edit:** Was able to make the edit and merge the PR. All looks good!
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
bug
Something isn't working
documentation
Improvements or additions to documentation
duplicate
This issue or pull request already exists
enhancement
New feature or request
good first issue
Good for newcomers
help wanted
Extra attention is needed
invalid
This doesn't seem right
question
Further information is requested
wontfix
This will not be worked on
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clyne
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: clyne/constexpr-to-string#2
No description provided.
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?