Skip to content

Flip some negated #if checks. NFC #25487

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

Merged
merged 1 commit into from
Oct 3, 2025
Merged

Flip some negated #if checks. NFC #25487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions src/jsifier.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,15 +601,7 @@ function(${args}) {
warn('To build in STANDALONE_WASM mode without a main(), use emcc --no-entry');
}
}
if (!RELOCATABLE) {
Copy link
Member

Choose a reason for hiding this comment

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

OTOH, "not relocatable == normal", and it's nice to have the normal path first? Then reading the more complex relocatable one after is easier.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one has annoyed me for a while. Every time I read I feel like have to do mental work for some reason.

Copy link
Member

Choose a reason for hiding this comment

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

If the negation here is annoying, how about var ABSOLUTE = !RELOCATABLE or some better name for "simple, absolute-addressed single file with no linking involved" - ?

(Perhaps "linked file", "relocated file", "flat file" - maybe there is a good term?)

That would avoid the negation, and also let us keep having the simple code first in each such if-else.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually in this case I like the if RELOCATABLE.

The code here is part of the undefined symbol handling. Its within a block and its already an error in most cases.

It reads like this:

if (is_undefined) {
   maybe_report_an_error();
   // Here we are continuing with the undefined symbol
   if (RELOCATABLE) {  // AKA dynamic linking
      create_a_dylink_stub();
   } else {
      create_an_abort_stub();
   } 
}

I think this is more logical that the other way around:

if (is_undefined) {
   maybe_report_an_error();
   // Here we are continuing with the undefined symbol
   if (!RELOCATABLE) {
      create_an_abort_stub();
   } else {
      create_a_dylink_stub();
   } 
}

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, I can live with this one. Reading the others again now.

// emit a stub that will fail at runtime
LibraryManager.library[symbol] = new Function(`abort('missing function: ${symbol}');`);
// We have already warned/errored about this function, so for the purposes of Closure use, mute all type checks
// regarding this function, marking ot a variadic function that can take in anything and return anything.
// (not useful to warn/error multiple times)
LibraryManager.library[symbol + '__docs'] = '/** @type {function(...*):?} */';
isStub = true;
} else {
if (RELOCATABLE) {
// Create a stub for this symbol which can later be replaced by the
// dynamic linker. If this stub is called before the symbol is
// resolved assert in debug builds or trap in release builds.
Expand All @@ -626,6 +618,14 @@ function(${args}) {
const functionBody = assertion + `return ${target}(...args);`;
LibraryManager.library[symbol] = new Function('...args', functionBody);
isStub = true;
} else {
// emit a stub that will fail at runtime
LibraryManager.library[symbol] = new Function(`abort('missing function: ${symbol}');`);
// We have already warned/errored about this function, so for the purposes of Closure use, mute all type checks
// regarding this function, marking ot a variadic function that can take in anything and return anything.
// (not useful to warn/error multiple times)
LibraryManager.library[symbol + '__docs'] = '/** @type {function(...*):?} */';
isStub = true;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just noticed the isStub = true; is repeated in both condition here too

}
}

Expand Down
6 changes: 3 additions & 3 deletions src/lib/libcore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2556,10 +2556,10 @@ function wrapSyscallFunction(x, library, isWasi) {
}

var isVariadic = !isWasi && t.includes(', varargs');
#if SYSCALLS_REQUIRE_FILESYSTEM == 0
var canThrow = false;
#else
#if SYSCALLS_REQUIRE_FILESYSTEM
var canThrow = library[x + '__nothrow'] !== true;
#else
var canThrow = false;
#endif

library[x + '__deps'] ??= [];
Expand Down
54 changes: 27 additions & 27 deletions src/lib/libemval.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,33 +279,7 @@ var LibraryEmVal = {
var argFromPtr = argTypes.map(type => type.readValueFromPointer.bind(type));
argCount--; // remove the extracted return type
#if !DYNAMIC_EXECUTION
var argN = new Array(argCount);
var invokerFunction = (handle, methodName, destructorsRef, args) => {
var offset = 0;
for (var i = 0; i < argCount; ++i) {
argN[i] = argFromPtr[i](args + offset);
offset += GenericWireTypeSize;
}
var rv;
switch (kind) {
case {{{ cDefs['internal::EM_INVOKER_KIND::FUNCTION'] }}}:
rv = Emval.toValue(handle).apply(null, argN);
break;
case {{{ cDefs['internal::EM_INVOKER_KIND::CONSTRUCTOR'] }}}:
rv = Reflect.construct(Emval.toValue(handle), argN);
break;
case {{{ cDefs['internal::EM_INVOKER_KIND::CAST'] }}}:
// no-op, just return the argument
rv = argN[0];
break;
case {{{ cDefs['internal::EM_INVOKER_KIND::METHOD'] }}}:
rv = Emval.toValue(handle)[getStringOrSymbol(methodName)](...argN);
break;
}
return emval_returnValue(toReturnWire, destructorsRef, rv);
};
#else
#if DYNAMIC_EXECUTION
var captures = {'toValue': Emval.toValue};
var args = argFromPtr.map((argFromPtr, i) => {
var captureName = `argFromPtr${i}`;
Expand Down Expand Up @@ -339,6 +313,32 @@ ${functionBody}
}`;
var invokerFunction = new Function(Object.keys(captures), functionBody)(...Object.values(captures));
#else
var argN = new Array(argCount);
var invokerFunction = (handle, methodName, destructorsRef, args) => {
var offset = 0;
for (var i = 0; i < argCount; ++i) {
argN[i] = argFromPtr[i](args + offset);
offset += GenericWireTypeSize;
}
var rv;
switch (kind) {
case {{{ cDefs['internal::EM_INVOKER_KIND::FUNCTION'] }}}:
rv = Emval.toValue(handle).apply(null, argN);
break;
case {{{ cDefs['internal::EM_INVOKER_KIND::CONSTRUCTOR'] }}}:
rv = Reflect.construct(Emval.toValue(handle), argN);
break;
case {{{ cDefs['internal::EM_INVOKER_KIND::CAST'] }}}:
// no-op, just return the argument
rv = argN[0];
break;
case {{{ cDefs['internal::EM_INVOKER_KIND::METHOD'] }}}:
rv = Emval.toValue(handle)[getStringOrSymbol(methodName)](...argN);
break;
}
return emval_returnValue(toReturnWire, destructorsRef, rv);
};
#endif
var functionName = `methodCaller<(${argTypes.map(t => t.name)}) => ${retType.name}>`;
return emval_addMethodCaller(createNamedFunction(functionName, invokerFunction));
Expand Down
10 changes: 5 additions & 5 deletions src/lib/libsdl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1743,17 +1743,17 @@ var LibrarySDL = {
// We actually do the whole screen in Unlock...
},

#if !ASYNCIFY
#if ASYNCIFY
SDL_Delay__deps: ['emscripten_sleep'],
SDL_Delay__async: true,
SDL_Delay: (delay) => _emscripten_sleep(delay),
#else
SDL_Delay: (delay) => {
if (!ENVIRONMENT_IS_WORKER) abort('SDL_Delay called on the main thread! Potential infinite loop, quitting. (consider building with async support like ASYNCIFY)');
// horrible busy-wait, but in a worker it at least does not block rendering
var now = Date.now();
while (Date.now() - now < delay) {}
},
#else
SDL_Delay__deps: ['emscripten_sleep'],
SDL_Delay__async: true,
SDL_Delay: (delay) => _emscripten_sleep(delay),
#endif

SDL_WM_SetCaption__proxy: 'sync',
Expand Down
6 changes: 3 additions & 3 deletions src/postamble_minimal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,10 +258,10 @@ WebAssembly.instantiate(Module['wasm'], imports).then(/** @suppress {missingProp
wasmExports = applySignatureConversions(wasmExports);
#endif

#if !DECLARE_ASM_MODULE_EXPORTS
exportWasmSymbols(wasmExports);
#else
#if DECLARE_ASM_MODULE_EXPORTS
assignWasmExports(wasmExports);
#else
exportWasmSymbols(wasmExports);
#endif

#if !IMPORTED_MEMORY
Expand Down