8000 fix slice low/high index shadowing variables in vm by ipsusila · Pull Request #394 · d5/tengo · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix slice low/high index shadowing variables in vm #394

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

Merged
merged 1 commit into from
Sep 26, 2022
Merged

fix slice low/high index shadowing variables in vm #394

merged 1 commit into from
Sep 26, 2022

Conversation

ipsusila
Copy link
Contributor

Variables low and high are being shadowed. Therefore, when slice index is invalid (not an integer), the error message does not gives correct invalid type hint, and always return Runtime Error: invalid slice index type: int

@geseq
Copy link
Collaborator
geseq commented Sep 26, 2022

@ipsusila did you mean to add the encode/decode to this PR?

@ipsusila
Copy link
Contributor Author

@geseq I'm sorry that the PR is confusing. After I submitting the PR which is the fix for low/high variables, I add commit which includes Encode/Decode to Compiled to forked repo. I did't realised that the later commit also included in this PR.

For Encode/Decode feature, I need your suggestions, wether it's acceptable to add such feature. If yes, should I separate the PR, or can we simply change the title and submit both PR?

@geseq
Copy link
Collaborator
geseq commented Sep 26, 2022

No worries at all.

Ideally please move it to a separate PR. I’ll take a look at it later.

@ipsusila
Copy link
Contributor Author

Thank you @geseq I revert Encode/Decode related commit from PR. For Encode/decode, later I will submit the PR.

@geseq geseq merged commit a419bfc into d5:master Sep 26, 2022
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.

2 participants
0