FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix signed delta by fncnt · Pull Request #36 · softdevteam/packedvec · GitHub

/ packedvec Public

Fix signed delta - #36

Merged
bors[bot] merged 2 commits into
softdevteam:masterfrom
fncnt:fix-signed-delta
Aug 4, 2021
ConversationCommits (2)ChecksFiles changed
Merged

Fix signed delta#36
bors[bot] merged 2 commits into
softdevteam:masterfrom
fncnt:fix-signed-delta

Conversation

fncnt commented Aug 4, 2021

Copy link
Copy Markdown
Contributor

Currently, constructing a new PackedVec from a vector like vec![-2, -1, 0] does not work properly. The reason lies in the computation of delta for both negative min and max arguments.
E.g. delta(-2, -1) is abs(max) - abs(min) which, in this case, overflows since abs(-1) - abs(-2) == -1 is negative but StorageT is bound to be Unsigned.

This PR addresses this by computing delta as abs(max - min) instead.
Corresponding test cases are added as well. From what I can tell, inv_delta was already working correctly as illustrated by a new test case.
Please let me know if I missed important details.

ltratt commented Aug 4, 2021

Copy link
Copy Markdown
Member

Very well spotted! This looks good as-is. Let's see if Travis succeeds (if it doesn't, I might convert this repo to use bors, but that's my problem, not yours!).

fncnt commented Aug 4, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thanks!

EDIT: btw, Travis has moved its domain so I think it won't run at all currently.

ltratt commented Aug 4, 2021

Copy link
Copy Markdown
Member

We've now enabled bors/buildbot for this repo. Could you rebase this PR against master and force push? Once that's done, we can invoke bors. [Sorry for forcing this extra step on you. I should have enabled bors/buildbot ages ago on this repository, but a combination of laziness and stupidity meant that I didn't do so!]

fncnt force-pushed the fix-signed-delta branch from a0f1dce to b2f56a0 Compare August 4, 2021 11:36

fncnt commented Aug 4, 2021

Copy link
Copy Markdown
Contributor Author

No problem, done! Just let me know if there's anything else.

ltratt commented Aug 4, 2021

Copy link
Copy Markdown
Member

bors r+

bors Bot added a commit that referenced this pull request Aug 4, 2021
Merge #36
7c83bfe 
36: Fix signed delta r=ltratt a=fncnt



Co-authored-by: fncnt <github@vncnt.eu>

bors Bot commented Aug 4, 2021

Copy link
Copy Markdown
Contributor

Build failed:

fncnt force-pushed the fix-signed-delta branch from b2f56a0 to 11d08d9 Compare August 4, 2021 13:21

fncnt commented Aug 4, 2021

Copy link
Copy Markdown
Contributor Author

Sorry, forgot to cargo fmt (done and squashed now).

ltratt commented Aug 4, 2021

Copy link
Copy Markdown
Member

bors r+

ltratt commented Aug 4, 2021

Copy link
Copy Markdown
Member

Hmm, bors seems stuck.

bors r-

ltratt commented Aug 4, 2021

Copy link
Copy Markdown
Member

bors ping

bors Bot commented Aug 4, 2021

Copy link
Copy Markdown
Contributor

pong

ltratt commented Aug 4, 2021

Copy link
Copy Markdown
Member

bors r+

bors Bot commented Aug 4, 2021

Copy link
Copy Markdown
Contributor

Build succeeded:

bors Bot merged commit b98423a into softdevteam:master Aug 4, 2021

ltratt commented Aug 4, 2021

Copy link
Copy Markdown
Member

Thanks for this! A new release of packedvec will be up shortly (once #38 merges).

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ltratt ltratt 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.

2 participants

Back | FazBrowse Home | New Git URL