Skip to content

Commit

Permalink
Fix closure warnings in JS library code
Browse files Browse the repository at this point in the history
For some reason closure doesn't report these real issues in the current
configuration but they show up in the `MODULARIZE_INSTANCE`
configuration that I'm working on.

Most of these issues consist of adding default parameters to functions
declared on objects such as `FS`.

In addition there are a couple of other changes that are all real fixes:

- ErrnoError unified between old FS and wasm FS.  This should have been
  done as part of emscripten-core#21149
- Default initializer for `canvasResizedCallbackTargetThread` removed
  since `JSEvents.getTargetThreadForEventCallback()` will always return
  undefined when called with no arguments
- Extra arguments to copyIndexedColorData removed in library_sdl.js
  since it only ever called with one arg.
- Sorting callback for JSEvents.deferredCalls.sort updated to correctly
  return number rather than a boolean.

This change is basically code size neutral with a few tests coming in
a byte or two heavier and others a byte or two lighter.
  • Loading branch information
sbc100 committed Jun 3, 2024
1 parent 3de1b90 commit 18fac74
Show file tree
Hide file tree
Showing 14 changed files with 97 additions and 107 deletions.
7 changes: 5 additions & 2 deletions src/library_browser.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ var LibraryBrowser = {
timingValue: 0,
currentFrameNumber: 0,
queue: [],
#if USE_CLOSURE_COMPILER
expectedBlockers: 0,
#endif
pause() {
Browser.mainLoop.scheduler = null;
// Incrementing this signals the previous main loop that it's now become old, and it must return.
Expand Down Expand Up @@ -628,7 +631,7 @@ var LibraryBrowser = {
Browser.resizeListeners.forEach((listener) => listener(canvas.width, canvas.height));
},

setCanvasSize(width, height, noUpdates) {
setCanvasSize(width, height, noUpdates = false) {
var canvas = Module['canvas'];
Browser.updateCanvasDimensions(canvas, width, height);
if (!noUpdates) Browser.updateResizeListeners();
Expand Down Expand Up @@ -658,7 +661,7 @@ var LibraryBrowser = {
Browser.updateResizeListeners();
},

updateCanvasDimensions(canvas, wNative, hNative) {
updateCanvasDimensions(canvas, wNative = 0, hNative = 0) {
if (wNative && hNative) {
canvas.widthNative = wNative;
canvas.heightNative = hNative;
Expand Down
59 changes: 14 additions & 45 deletions src/library_fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ FS.staticInit();` +
devices: {},
streams: [],
nextInode: 1,
nameTable: null,
currentPath: '/',
initialized: false,
// Whether we are currently ignoring permissions. Useful when preparing the
Expand All @@ -62,40 +61,10 @@ FS.staticInit();` +
#if FS_DEBUG
trackingDelegate: {},
#endif
ErrnoError: null, // set during init
genericErrors: {},
filesystems: null,
syncFSRequests: 0, // we warn if there are multiple in flight at once

#if ASSERTIONS
ErrnoError: class extends Error {
#else
ErrnoError: class {
#endif
// We set the `name` property to be able to identify `FS.ErrnoError`
// - the `name` is a standard ECMA-262 property of error objects. Kind of good to have it anyway.
// - when using PROXYFS, an error can come from an underlying FS
// as different FS objects have their own FS.ErrnoError each,
// the test `err instanceof FS.ErrnoError` won't detect an error coming from another filesystem, causing bugs.
// we'll use the reliable test `err.name == "ErrnoError"` instead
constructor(errno) {
#if ASSERTIONS
super(ERRNO_MESSAGES[errno]);
#endif
// TODO(sbc): Use the inline member declaration syntax once we
// support it in acorn and closure.
this.name = 'ErrnoError';
this.errno = errno;
#if ASSERTIONS
for (var key in ERRNO_CODES) {
if (ERRNO_CODES[key] === errno) {
this.code = key;
break;
}
}
#endif
}
},
ErrnoError: ErrnoError,

FSStream: class {
constructor() {
Expand Down Expand Up @@ -304,7 +273,7 @@ FS.staticInit();` +
// if we failed to find it in the cache, call into the VFS
return FS.lookup(parent, name);
},
createNode(parent, name, mode, rdev) {
createNode(parent, name, mode, rdev = 0) {
#if ASSERTIONS
assert(typeof parent == 'object')
#endif
Expand Down Expand Up @@ -673,13 +642,13 @@ FS.staticInit();` +
return parent.node_ops.mknod(parent, name, mode, dev);
},
// helpers to create specific types of nodes
create(path, mode) {
create(path, mode = undefined) {
mode = mode !== undefined ? mode : 438 /* 0666 */;
mode &= {{{ cDefs.S_IALLUGO }}};
mode |= {{{ cDefs.S_IFREG }}};
return FS.mknod(path, mode, 0);
},
mkdir(path, mode) {
mkdir(path, mode = undefined) {
mode = mode !== undefined ? mode : 511 /* 0777 */;
mode &= {{{ cDefs.S_IRWXUGO }}} | {{{ cDefs.S_ISVTX }}};
mode |= {{{ cDefs.S_IFDIR }}};
Expand All @@ -691,7 +660,7 @@ FS.staticInit();` +
return FS.mknod(path, mode, 0);
},
// Creates a whole directory tree chain if it doesn't yet exist
mkdirTree(path, mode) {
mkdirTree(path, mode = undefined) {
var dirs = path.split('/');
var d = '';
for (var i = 0; i < dirs.length; ++i) {
Expand All @@ -704,7 +673,7 @@ FS.staticInit();` +
}
}
},
mkdev(path, mode, dev) {
mkdev(path, mode, dev = undefined) {
if (typeof dev == 'undefined') {
dev = mode;
mode = 438 /* 0666 */;
Expand Down Expand Up @@ -912,7 +881,7 @@ FS.staticInit();` +
}
return PATH_FS.resolve(FS.getPath(link.parent), link.node_ops.readlink(link));
},
stat(path, dontFollow) {
stat(path, dontFollow = false) {
var lookup = FS.lookupPath(path, { follow: !dontFollow });
var node = lookup.node;
if (!node) {
Expand All @@ -926,7 +895,7 @@ FS.staticInit();` +
lstat(path) {
return FS.stat(path, true);
},
chmod(path, mode, dontFollow) {
chmod(path, mode, dontFollow = false) {
var node;
if (typeof path == 'string') {
var lookup = FS.lookupPath(path, { follow: !dontFollow });
Expand All @@ -949,7 +918,7 @@ FS.staticInit();` +
var stream = FS.getStreamChecked(fd);
FS.chmod(stream.node, mode);
},
chown(path, uid, gid, dontFollow) {
chown(path, uid, gid, dontFollow = false) {
var node;
if (typeof path == 'string') {
var lookup = FS.lookupPath(path, { follow: !dontFollow });
Expand Down Expand Up @@ -1015,7 +984,7 @@ FS.staticInit();` +
timestamp: Math.max(atime, mtime)
});
},
open(path, flags, mode) {
open(path, flags, mode = undefined) {
if (path === "") {
throw new FS.ErrnoError({{{ cDefs.ENOENT }}});
}
Expand Down Expand Up @@ -1193,7 +1162,7 @@ FS.staticInit();` +
#endif
return bytesRead;
},
write(stream, buffer, offset, length, position, canOwn) {
write(stream, buffer, offset, length, position = undefined, canOwn = false) {
#if ASSERTIONS
assert(offset >= 0);
#endif
Expand Down Expand Up @@ -1466,7 +1435,7 @@ FS.staticInit();` +
#endif
};
},
init(input, output, error) {
init(input = undefined, output = undefined, error = undefined) {
#if ASSERTIONS
assert(!FS.init.initialized, 'FS.init was previously called. If you want to initialize later with custom parameters, remove any earlier calls (note that one is automatically added to the generated code)');
#endif
Expand Down Expand Up @@ -1505,7 +1474,7 @@ FS.staticInit();` +
}
return ret.object;
},
analyzePath(path, dontResolveLastLink) {
analyzePath(path, dontResolveLastLink = false) {
// operate from within the context of the symlink's target
try {
var lookup = FS.lookupPath(path, { follow: !dontResolveLastLink });
Expand Down Expand Up @@ -1576,7 +1545,7 @@ FS.staticInit();` +
FS.chmod(node, mode);
}
},
createDevice(parent, name, input, output) {
createDevice(parent, name, input, output = undefined) {
var path = PATH.join2(typeof parent == 'string' ? parent : FS.getPath(parent), name);
var mode = FS_getMode(!!input, !!output);
if (!FS.createDevice.major) FS.createDevice.major = 64;
Expand Down
32 changes: 31 additions & 1 deletion src/library_fs_shared.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,36 @@
* SPDX-License-Identifier: MIT
*/

#if ASSERTIONS
class ErrnoError extends Error {
#else
class ErrnoError {
#endif
// We set the `name` property to be able to identify `FS.ErrnoError`
// - the `name` is a standard ECMA-262 property of error objects. Kind of good to have it anyway.
// - when using PROXYFS, an error can come from an underlying FS
// as different FS objects have their own FS.ErrnoError each,
// the test `err instanceof FS.ErrnoError` won't detect an error coming from another filesystem, causing bugs.
// we'll use the reliable test `err.name == "ErrnoError"` instead
constructor(errno) {
#if ASSERTIONS
super(ERRNO_MESSAGES[errno]);
#endif
// TODO(sbc): Use the inline member declaration syntax once we
// support it in acorn and closure.
this.name = 'ErrnoError';
this.errno = errno;
#if ASSERTIONS
for (var key in ERRNO_CODES) {
if (ERRNO_CODES[key] === errno) {
this.code = key;
break;
}
}
#endif
}
}

addToLibrary({
$preloadPlugins: "{{{ makeModuleReceiveExpr('preloadPlugins', '[]') }}}",

Expand Down Expand Up @@ -50,7 +80,7 @@ addToLibrary({
'$FS_handledByPreloadPlugin',
#endif
],
$FS_createPreloadedFile: (parent, name, url, canRead, canWrite, onload, onerror, dontCreateFile, canOwn, preFinish) => {
$FS_createPreloadedFile: (parent, name, url, canRead, canWrite, onload, onerror, dontCreateFile, canOwn = false, preFinish = undefined) => {
// TODO we should allow people to just pass in a complete filename instead
// of parent and name being that we just join them anyways
var fullname = name ? PATH_FS.resolve(PATH.join2(parent, name)) : parent;
Expand Down
5 changes: 2 additions & 3 deletions src/library_glemu.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ var LibraryGLEmulation = {
fogStart: 0,
fogEnd: 1,
fogDensity: 1.0,
fogColor: null,
fogMode: 0x800, // GL_EXP
fogEnabled: false,

Expand Down Expand Up @@ -2127,7 +2126,7 @@ var LibraryGLEmulation = {
return renderer;
},

createRenderer(renderer) {
createRenderer() {
var useCurrProgram = !!GL.currProgram;
var hasTextures = false;
for (var i = 0; i < GLImmediate.MAX_TEXTURES; i++) {
Expand Down Expand Up @@ -2995,7 +2994,7 @@ var LibraryGLEmulation = {
}
},

flush(numProvidedIndexes, startIndex = 0, ptr = 0) {
flush(numProvidedIndexes = 0, startIndex = 0, ptr = 0) {
#if ASSERTIONS
assert(numProvidedIndexes >= 0 || !numProvidedIndexes);
#endif
Expand Down
16 changes: 7 additions & 9 deletions src/library_html5.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,7 @@ var LibraryHTML5 = {
return true;
}
// Test if the given call was already queued, and if so, don't add it again.
for (var i in JSEvents.deferredCalls) {
var call = JSEvents.deferredCalls[i];
for (var call of JSEvents.deferredCalls) {
if (call.targetFunction == targetFunction && arraysHaveEqualContent(call.argsList, argsList)) {
return;
}
Expand All @@ -106,7 +105,10 @@ var LibraryHTML5 = {
argsList
});

JSEvents.deferredCalls.sort((x,y) => x.precedence < y.precedence);
// sortcallback(x, y):
// A negative value indicates that x should come before y.
// A positive value indicates that x should come after y.
JSEvents.deferredCalls.sort((x,y) => y.precedence - x.precedence);
},

// Erases all deferred calls to the given target function from the queue list.
Expand Down Expand Up @@ -151,10 +153,9 @@ var LibraryHTML5 = {
// Removes all event handlers on the given DOM element of the given type.
// Pass in eventTypeString == undefined/null to remove all event handlers
// regardless of the type.
removeAllHandlersOnTarget: (target, eventTypeString) => {
removeAllHandlersOnTarget: (target) => {
for (var i = 0; i < JSEvents.eventHandlers.length; ++i) {
if (JSEvents.eventHandlers[i].target == target &&
(!eventTypeString || eventTypeString == JSEvents.eventHandlers[i].eventTypeString)) {
if (JSEvents.eventHandlers[i].target) {
JSEvents._removeHandler(i--);
}
}
Expand Down Expand Up @@ -1512,9 +1513,6 @@ var LibraryHTML5 = {
filteringMode: {{{ makeGetValue('fullscreenStrategy', C_STRUCTS.EmscriptenFullscreenStrategy.filteringMode, 'i32') }}},
canvasResizedCallback: {{{ makeGetValue('fullscreenStrategy', C_STRUCTS.EmscriptenFullscreenStrategy.canvasResizedCallback, 'i32') }}},
canvasResizedCallbackUserData: {{{ makeGetValue('fullscreenStrategy', C_STRUCTS.EmscriptenFullscreenStrategy.canvasResizedCallbackUserData, 'i32') }}},
#if PTHREADS
canvasResizedCallbackTargetThread: JSEvents.getTargetThreadForEventCallback(),
#endif
target,
softFullscreen: true
};
Expand Down
8 changes: 4 additions & 4 deletions src/library_nodefs.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@ addToLibrary({
#if ASSERTIONS
assert(ENVIRONMENT_IS_NODE);
#endif
return NODEFS.createNode(null, '/', NODEFS.getMode(mount.opts.root), 0);
return NODEFS.createNode(null, '/', NODEFS.getMode(mount.opts.root));
},
createNode(parent, name, mode, dev) {
createNode(parent, name, mode) {
if (!FS.isDir(mode) && !FS.isFile(mode) && !FS.isLink(mode)) {
throw new FS.ErrnoError({{{ cDefs.EINVAL }}});
}
Expand Down Expand Up @@ -176,7 +176,7 @@ addToLibrary({
return NODEFS.createNode(parent, name, mode);
},
mknod(parent, name, mode, dev) {
var node = NODEFS.createNode(parent, name, mode, dev);
var node = NODEFS.createNode(parent, name, mode);
// create the backing node for this in the fs root as well
var path = NODEFS.realPath(node);
NODEFS.tryFSOperation(() => {
Expand Down Expand Up @@ -277,7 +277,7 @@ addToLibrary({
return { ptr, allocated: true };
},
msync(stream, buffer, offset, length, mmapFlags) {
NODEFS.stream_ops.write(stream, buffer, 0, length, offset, false);
NODEFS.stream_ops.write(stream, buffer, 0, length, offset);
// should we check if bytesWritten and length are the same?
return 0;
}
Expand Down
2 changes: 1 addition & 1 deletion src/library_openal.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ var LibraryOpenAL = {
// represents the queue of buffers scheduled for physical playback. These two queues are
// distinct because of the differing semantics of OpenAL and web audio. Some changes
// to OpenAL parameters, such as pitch, may require the web audio queue to be flushed and rescheduled.
scheduleSourceAudio: (src, lookahead) => {
scheduleSourceAudio: (src, lookahead = false) => {
// See comment on scheduleContextAudio above.
if (Browser.mainLoop.timingMode === {{{ cDefs.EM_TIMING_RAF }}} && document['visibilityState'] != 'visible') {
return;
Expand Down
Loading

0 comments on commit 18fac74

Please sign in to comment.