Skip to content

Make more use of allocateUTF8 helper. NFC #19064

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 27, 2023
Merged

Make more use of allocateUTF8 helper. NFC #19064

merged 1 commit into from
Mar 27, 2023

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Mar 24, 2023

No description provided.

@sbc100 sbc100 mentioned this pull request Mar 24, 2023
Closed
@sbc100 sbc100 force-pushed the use_allocate_utf8 branch from 1605839 to 984241f Compare March 24, 2023 19:32
@sbc100 sbc100 requested review from juj and kripken March 24, 2023 19:32
@sbc100 sbc100 force-pushed the use_allocate_utf8 branch 2 times, most recently from 0804c5b to d94c606 Compare March 24, 2023 20:49
@sbc100 sbc100 requested a review from tlively March 24, 2023 21:56
@sbc100 sbc100 force-pushed the use_allocate_utf8 branch from d94c606 to 5122820 Compare March 24, 2023 23:32
@sbc100 sbc100 enabled auto-merge (squash) March 25, 2023 01:53
tlively
tlively reviewed Mar 27, 2023
View reviewed changes
src/library.js
// We never free the return values of this function so we need to allocate
// using builtin_malloc to avoid LSan reporting these as leaks.
emscripten_get_compiler_setting__noleakcheck: true,
#if RETAIN_COMPILER_SETTINGS
Copy link
Member

@tlively tlively Mar 27, 2023

Choose a reason for hiding this comment

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

I've never heard of this setting before. Does anyone use it?

Copy link
Collaborator Author

@sbc100 sbc100 Mar 27, 2023

Choose a reason for hiding this comment

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

It was originally added by @juj I believe so I assume they are using it for something. We use it in a few places in our test code too IIRC.

Copy link
Collaborator

@juj juj Mar 28, 2023

Choose a reason for hiding this comment

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

I never used this function, but it was added after there were more than one report from Emscripten users that they wanted to have the ability to read this setting. Internet Archive's DosBox project may have been one if memory serves, they had a need to be able to dynamically check with builds which options were used, since they were updating games vs engines at different cadences.

If I recall, I was actually reluctant to add this setting because of the addition to preamble.js that it would incorporate, which is why it went behind a build flag so code size would not unconditionally grow for all users.

Ideally it would be great to fully move this to a library file in the same fashion you attacked some of the other functions from the preambles.

Copy link
Collaborator

@juj juj Mar 28, 2023

Choose a reason for hiding this comment

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

Searching GitHub for uses of emscripten_get_compiler_setting does find some hits:

@sbc100 sbc100 force-pushed the use_allocate_utf8 branch from 5122820 to 8a08504 Compare March 27, 2023 16:56
@sbc100 sbc100 requested a review from tlively March 27, 2023 18:40
sbc100 added a commit that referenced this pull request Mar 27, 2023
@sbc100
stringToNewUTF8 moved to library_legacy.js.
110b21f 
All remaining internal usages converted to `allocateUTF8`.  These
functions do that same thing and were made into aliases in #19064.

`stringToNewUTF8` was added in 443447b and has fewer uses than
`allocateUTF8`.

`allocateUTF8` was added in 5174d2e.
@sbc100 sbc100 mentioned this pull request Mar 27, 2023
Closed
@sbc100 sbc100 force-pushed the use_allocate_utf8 branch from 8a08504 to fba10b7 Compare March 27, 2023 21:37
@sbc100 sbc100 merged commit 79a7ca9 into main Mar 27, 2023
@sbc100 sbc100 deleted the use_allocate_utf8 branch March 27, 2023 22:44
sbc100 added a commit that referenced this pull request Mar 27, 2023
@sbc100
stringToNewUTF8 moved to library_legacy.js.
1076c01 
All remaining internal usages converted to `allocateUTF8`.  These
functions do that same thing and were made into aliases in #19064.

`stringToNewUTF8` was added in 443447b and has fewer uses than
`allocateUTF8`.

`allocateUTF8` was added in 5174d2e.
sbc100 added a commit that referenced this pull request Mar 28, 2023
@sbc100
Deprecate allocateUTF8 in favor of stringToNewUTF8
7fa1997 
Followup to #19064
@sbc100 sbc100 mentioned this pull request Mar 28, 2023
Merged
sbc100 added a commit that referenced this pull request Mar 29, 2023
@sbc100
Deprecate allocateUTF8 in favor of stringToNewUTF8 (#19089)
4e15675 
Followup to #19064
RReverser pushed a commit that referenced this pull request Mar 29, 2023
@sbc100 @RReverser
Make more use of allocateUTF8 helper. NFC (#19064)
bbda71b
RReverser pushed a commit that referenced this pull request Mar 29, 2023
@sbc100 @RReverser
Deprecate allocateUTF8 in favor of stringToNewUTF8 (#19089)
63ab935 
Followup to #19064
@MADCATXXX MADCATXXX mentioned this pull request Apr 17, 2023
Merged
sbc100 added a commit to sbc100/emscripten that referenced this pull request Sep 18, 2024
@sbc100
Remove outdated comment. NFC
0c8e89d 
The comment pertains to code that was removed in emscripten-core#19064.
@sbc100 sbc100 mentioned this pull request Sep 18, 2024
Merged
sbc100 added a commit that referenced this pull request Sep 18, 2024
@sbc100
Remove outdated comment. NFC (#22586)
7e8b4c0 
The comment pertains to code that was removed in #19064.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tlively tlively tlively approved these changes

@kripken kripken Awaiting requested review from kripken

@juj juj Awaiting requested review from juj

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sbc100 @juj @tlively