Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Add / adapt upstream fixes #211

Merged
merged 3 commits into from
Dec 14, 2023
Merged

Add / adapt upstream fixes #211

merged 3 commits into from
Dec 14, 2023

Conversation

saghul
Copy link
Contributor

@saghul saghul commented Dec 14, 2023

No description provided.

@saghul saghul requested a review from bnoordhuis December 14, 2023 09:29
@bnoordhuis
Copy link
Contributor

I think we don't need -fwrapv, the ubsan buildbot should catch (and, in the past, has caught) instances of signed integer overflow.

@saghul
Copy link
Contributor Author

saghul commented Dec 14, 2023

I think we don't need -fwrapv, the ubsan buildbot should catch (and, in the past, has caught) instances of signed integer overflow.

Does it hurt to have it? I don't mind dropping that commit.

@saghul
Copy link
Contributor Author

saghul commented Dec 14, 2023

@bnoordhuis Added one last commit, PTAL.

Note I omitted this part bellard/quickjs@4bb8c35#diff-45f1ae674139f993bf8a99c382c1ba4863272a6fec2f492d76d7ff1b2cfcfbe2R31978 since we defined DUMP_BYTECODE 64 ourselves. Shall I rename our addition to 128 and keep the upstream one as 64?

@saghul saghul marked this pull request as ready for review December 14, 2023 09:57
@bnoordhuis
Copy link
Contributor

Does it hurt to have it?

It can. It inhibits some compiler optimizations around loops.

Shall I rename our addition to 128 and keep the upstream one as 64?

Sure, seems fine to me.

@saghul
Copy link
Contributor Author

saghul commented Dec 14, 2023

Does it hurt to have it?

It can. It inhibits some compiler optimizations around loops.

Shall I rename our addition to 128 and keep the upstream one as 64?

Sure, seems fine to me.

Ack, fixed both!

@saghul saghul merged commit e581286 into master Dec 14, 2023
@saghul saghul deleted the sync branch December 14, 2023 10:49
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis 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
@saghul @bnoordhuis