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(codegen/regex): allow vec growth on parse #405

Merged
merged 6 commits into from
Aug 8, 2024

Conversation

LeoDog896
Copy link
Contributor

@LeoDog896 LeoDog896 commented Jul 24, 2024

Fixes #389. This is less efficient, but it's on codegen.

@jeertmans
Copy link
Collaborator

Hello @LeoDog896, thank you for your contribution!

Can you motivate why we need to have a possibly growing vector instead?
I see where the error might come from, but that would be great to document / explain it a little better, as it goes against the previous code comments left by the author of Logos.

@LeoDog896
Copy link
Contributor Author

LeoDog896 commented Jul 25, 2024

Yep! I'll write this in a comment soon, but the original vec allocation never accounted for the size of the literals - it always assumed every ropable element would only have one item, and each item would be four Unicode codepoints long; since literals can be longer than four Unicode points, if there was more than eight tokens (two being the minimum size to create a concat literal), it would cause that integer overflow error.

The only way to get that size would be to walk through all the elements in the concat node and add up the lengths of all of these elements.

@jeertmans
Copy link
Collaborator

Sorry for the late review @LeoDog896, I was on holidays. This looks great and clear to me.

Before I merge this, did you check that long_concat_389 fails without your fix?

@jeertmans jeertmans added the bug Something isn't working label Aug 7, 2024
@LeoDog896
Copy link
Contributor Author

Sorry for the late review @LeoDog896, I was on holidays. This looks great and clear to me.

Before I merge this, did you check that long_concat_389 fails without your fix?

image

On current master, with only long_concat_389 added, it fails 👍

@jeertmans
Copy link
Collaborator

Looks good, thanks!

@jeertmans jeertmans merged commit 0ce5806 into maciejhirsz:master Aug 8, 2024
18 checks passed
akrantz01 referenced this pull request in akrantz01/antsi Sep 19, 2024
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [logos](https://logos.maciej.codes/)
([source](https://redirect.github.com/maciejhirsz/logos)) | dependencies
| patch | `0.14.1` -> `0.14.2` |

---

### Release Notes

<details>
<summary>maciejhirsz/logos (logos)</summary>

###
[`v0.14.2`](https://redirect.github.com/maciejhirsz/logos/releases/tag/v0.14.2):
- Optional `forbid_unsafe` feature, fuzzing, book, and more!

[Compare
Source](https://redirect.github.com/maciejhirsz/logos/compare/v0.14.1...v0.14.2)

#### What's Changed

- chore(book): added link to Rust's reference by
[@&#8203;CommanderStorm](https://redirect.github.com/CommanderStorm) in
[https://github.com/maciejhirsz/logos/pull/411](https://redirect.github.com/maciejhirsz/logos/pull/411)
- feat: impl Source for T: Deref in no_std by
[@&#8203;yjhmelody](https://redirect.github.com/yjhmelody) in
[https://github.com/maciejhirsz/logos/pull/406](https://redirect.github.com/maciejhirsz/logos/pull/406)
- fix(codegen/regex): allow vec growth on parse by
[@&#8203;LeoDog896](https://redirect.github.com/LeoDog896) in
[https://github.com/maciejhirsz/logos/pull/405](https://redirect.github.com/maciejhirsz/logos/pull/405)
- test: basic fuzzing by
[@&#8203;LeoDog896](https://redirect.github.com/LeoDog896) in
[https://github.com/maciejhirsz/logos/pull/407](https://redirect.github.com/maciejhirsz/logos/pull/407)
- feat(lib): add `forbid_unsafe` feature to disable unsafe code by
[@&#8203;davidkern](https://redirect.github.com/davidkern) in
[https://github.com/maciejhirsz/logos/pull/413](https://redirect.github.com/maciejhirsz/logos/pull/413)
- chore(version): release v0.14.2 by
[@&#8203;jeertmans](https://redirect.github.com/jeertmans) in
[https://github.com/maciejhirsz/logos/pull/422](https://redirect.github.com/maciejhirsz/logos/pull/422)

#### New Contributors

- [@&#8203;CommanderStorm](https://redirect.github.com/CommanderStorm)
made their first contribution in
[https://github.com/maciejhirsz/logos/pull/411](https://redirect.github.com/maciejhirsz/logos/pull/411)
- [@&#8203;yjhmelody](https://redirect.github.com/yjhmelody) made their
first contribution in
[https://github.com/maciejhirsz/logos/pull/406](https://redirect.github.com/maciejhirsz/logos/pull/406)
- [@&#8203;davidkern](https://redirect.github.com/davidkern) made their
first contribution in
[https://github.com/maciejhirsz/logos/pull/413](https://redirect.github.com/maciejhirsz/logos/pull/413)

**Full Changelog**:
maciejhirsz/logos@v0.14.1...v0.14.2

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/akrantz01/antsi).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC44MC4wIiwidXBkYXRlZEluVmVyIjoiMzguODAuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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 this pull request may close these issues.

proc-macro derive panicked message: attempt to subtract with overflow
2 participants