Skip to content

Make more native constants in JS library files. NFC - #28199

Open
sbc100 wants to merge 1 commit into
emscripten-core:mainfrom
sbc100:computed_props
Open

sbc100 wants to merge 1 commit into
emscripten-core:mainfrom
sbc100:computed_props

Conversation

@sbc100

@sbc100 sbc100 commented Nov 6, 2026 •

Copy link
Copy Markdown
Collaborator

In particular, make use of computed property names where possible.

The small code size increase in WebGL builds (10 bytes in WebGL 1, 18
bytes in WebGL 2) is due to colorChannelsInGlTextureFormat using
computed property names ([GL_RGB - GL_DEPTH_COMPONENT]: 3), which
Closure folds to [5]:3 rather than 5:3 (2 extra bytes per entry).

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request refactors several library files (including libglemu.js, libglew.js, libglfw.js, libsdl.js, and libwebgl.js) to replace hardcoded magic numbers with symbolic constants defined in struct_info.json. However, in libglfw.js, several GLFW constants (such as GLFW_CURSOR and GLFW_RED_BITS) are accessed directly instead of through the cDefs object, which will lead to runtime ReferenceErrors. These constants must be prefixed with cDefs. to be correctly resolved.

Comment thread src/lib/libglfw.js Resolved
Comment thread src/lib/libglfw.js Resolved
@sbc100
sbc100 requested a review from kripken November 6, 2026 21:27
@sbc100
sbc100 marked this pull request as draft November 6, 2026 21:55
@sbc100 sbc100 changed the title Make more use of computed propertie names in JS library files. NFC Make more native constants in JS library files. NFC Nov 6, 2026
@sbc100
sbc100 force-pushed the computed_props branch 2 times, most recently from fad7194 to 1625de3 Compare November 6, 2026 22:38
@sbc100
sbc100 marked this pull request as ready for review November 6, 2026 22:38

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request replaces hardcoded numeric literals with symbolic constant names for GL, GLEW, GLFW, and SDL across several Emscripten JS library files, and updates the corresponding struct info metadata. However, the reviewer noted that referencing these constants directly in the JS scope will cause runtime ReferenceErrors and introduce runtime overhead. To resolve this, the reviewer recommends using compile-time preprocessor expansion (e.g., {{{ cDefs.CONSTANT_NAME }}}) to safely inline the constant values at compile-time.

Comment thread src/lib/libglemu.js Resolved
Comment thread src/lib/libglemu.js Resolved
Comment thread src/lib/libglew.js Resolved
Comment thread src/lib/libglfw.js Resolved
Comment thread src/lib/libglfw.js Resolved
Comment thread src/lib/libwebgl.js Resolved
In particular, make use of computed property names where possible.

@kripken kripken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm % questions

Comment thread src/lib/libglfw.js
break;
}
case 0x00033002: { // GLFW_STICKY_KEYS
case GLFW_STICKY_KEYS: {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This constant differs between glfw and glfw3:

system/include/GLFW/glfw3.h:#define GLFW_STICKY_KEYS            0x00033002
system/include/GL/glfw.h:#define GLFW_STICKY_KEYS          0x00030002

Which should it be here? It looks like it is right in struct_info, but I am a little unsure?

@@ -1,7 +1,7 @@
{
"a.out.js": 272465,
"a.out.js": 272483,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any idea why closure doesn't turn [5]:3 into 5:3?

Perhaps it is worth doing {{{ .. }}}: 3 in these cases to avoid a regression?

This branch has not been deployed

No deployments
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