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

fix: correctly offset vector input #910

Merged
merged 9 commits into from
Apr 20, 2023
Merged

fix: correctly offset vector input #910

merged 9 commits into from
Apr 20, 2023

Conversation

camsjams
Copy link
Contributor

@camsjams camsjams commented Apr 19, 2023

Aside from fixing the issue, to get there I had to write some tests and iron out the exact nuances. The final result looks simpler overall, but there were some fun bugs to kill as I changed the Vec data code.

I've added a lot of unit tests in different places as a result.

Issue

The pointer of the Vector data was wrong, caused by a bug when the index of the first vector param was not zero.

Solution

By always starting with the base offset, the bug is fixed.

These lines were the culprit, as this would only run if the first vector was also the first input:

if (byteIndex === 0 && byteIndex === paramIndex) {
  return baseVectorOffset;
}

Fixes #881

Note: I'd recommend also seeing the original PR here: #497 which includes a still accurate diagram.

@camsjams camsjams self-assigned this Apr 19, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 19, 2023

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements 95.05% 4643/4885
🟢 Branches
83.89% (-0.03% 🔻)
823/981
🟢 Functions 92.32% 890/964
🟢 Lines 95.07% 4440/4670
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / utilities.ts
100%
87.5% (-2.5% 🔻)
100% 100%

Test suite run success

794 tests passing in 115 suites.

Report generated by 🧪jest coverage report action from 80f9cc5

@camsjams camsjams enabled auto-merge (squash) April 19, 2023 06:48
Copy link
Member

@Dhaiwat10 Dhaiwat10 left a comment

Choose a reason for hiding this comment

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

Finally 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect encoding of Vec when it's not the first parameter in the function
3 participants