Skip to content
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.

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

Fix closure warnings in JS library code #21670

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
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) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it better perhaps to use a closure annotation here instead, which would not add code size? (As this is safe - it is just that closure flags it as a possible bug, but here it isn't.)

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 @@ -670,13 +639,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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does Closure optimize these = undefined away?

mode = mode !== undefined ? mode : 511 /* 0777 */;
mode &= {{{ cDefs.S_IRWXUGO }}} | {{{ cDefs.S_ISVTX }}};
mode |= {{{ cDefs.S_IFDIR }}};
Expand All @@ -688,7 +657,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 @@ -701,7 +670,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 @@ -906,7 +875,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 @@ -920,7 +889,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 @@ -943,7 +912,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 @@ -1009,7 +978,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 @@ -1187,7 +1156,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 @@ -1460,7 +1429,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 @@ -1499,7 +1468,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 @@ -1570,7 +1539,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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be JSEvents.eventHandlers[i].target == 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 @@ -53,9 +53,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 @@ -169,7 +169,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);
try {
Expand Down Expand Up @@ -320,7 +320,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
Loading