Skip to content

Remove redundant alias handling in JS compiler. NFC. #17420

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
Jul 12, 2022
Merged

Remove redundant alias handling in JS compiler. NFC. #17420

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
10 changes: 3 additions & 7 deletions src/jsifier.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,6 @@ function ${name}(${args}) {

const original = LibraryManager.library[ident];
let snippet = original;
let redirectedIdent = null;
const deps = LibraryManager.library[ident + '__deps'] || [];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reassignment of deps below was previously dead code so this const was OK prior to this change.

if (!Array.isArray(deps)) {
error(`JS library directive ${ident}__deps=${deps.toString()} is of type ${typeof deps}, but it should be an array!`);
Expand All @@ -313,9 +312,9 @@ function ${name}(${args}) {
if (target) {
// Redirection for aliases. We include the parent, and at runtime make ourselves equal to it.
// This avoid having duplicate functions with identical content.
redirectedIdent = snippet;
deps.push(snippet);
snippet = mangleCSymbolName(snippet);
const redirectedTarget = snippet;
deps.push(redirectedTarget);
snippet = mangleCSymbolName(redirectedTarget);
}
}
} else if (typeof snippet == 'object') {
Expand Down Expand Up @@ -349,9 +348,6 @@ function ${name}(${args}) {
}
}

if (redirectedIdent) {
deps = deps.concat(LibraryManager.library[redirectedIdent + '__deps'] || []);
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 was previously dead code since redirectedIdent was always undefined prior to this change. deps is declared as const which means this line would be runtime error if it ever ran.

The dependencies of the target are already handled by the deps.push(redirectedTarget); above.

}
if (VERBOSE) {
printErr(`adding ${finalName} and deps ${deps} : ` + (snippet + '').substr(0, 40));
}
Expand Down
75 changes: 6 additions & 69 deletions src/modules.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,75 +237,12 @@ global.LibraryManager = {
}
}

// apply synonyms. these are typically not speed-sensitive, and doing it
// this way makes it possible to not include hacks in the compiler
// (and makes it simpler to switch between SDL versions, fastcomp and non-fastcomp, etc.).
const lib = this.library;
libloop: for (const x in lib) {
if (!Object.prototype.hasOwnProperty.call(lib, x)) {
continue;
}
if (isJsLibraryConfigIdentifier(x)) {
const index = x.lastIndexOf('__');
const basename = x.slice(0, index);
if (!(basename in lib)) {
error(`Missing library element '${basename}' for library config '${x}'`);
}
continue;
}
if (typeof lib[x] == 'string') {
let target = x;
while (typeof lib[target] == 'string') {
// ignore code and variable assignments, aliases are just simple names
if (lib[target].search(/[=({; ]/) >= 0) continue libloop;
target = lib[target];
}
if (!isNaN(target)) continue; // This is a number, and so cannot be an alias target.
if (typeof lib[target] == 'undefined' || typeof lib[target] == 'function') {
// When functions are aliased, a signature for the function must be
// provided so that an efficient form of forwarding can be
// implemented.
function testStringType(sig) {
if (typeof lib[sig] != 'undefined' && typeof typeof lib[sig] != 'string') {
error(`${sig} should be a string! (was ${typeof lib[sig]})`);
}
}
const aliasSig = x + '__sig';
const targetSig = target + '__sig';
testStringType(aliasSig);
testStringType(targetSig);
if (typeof lib[aliasSig] == 'string' && typeof lib[targetSig] == 'string' && lib[aliasSig] != lib[targetSig]) {
error(`${aliasSig} (${lib[aliasSig]}) differs from ${targetSig} (${lib[targetSig]})`);
}

const sig = lib[aliasSig] || lib[targetSig];
if (typeof sig != 'string') {
error(`Function ${x} aliases to target function ${target}, but neither the alias or the target provide a signature. Please add a ${targetSig}: 'vifj...' annotation or a ${aliasSig}: 'vifj...' annotation to describe the type of function forwarding that is needed!`);
}

// If only one of the target or the alias specifies a sig then copy
// this signature to the other.
if (!lib[aliasSig]) {
lib[aliasSig] = lib[targetSig];
} else if (!lib[targetSig]) {
lib[targetSig] = lib[aliasSig];
}

if (typeof lib[target] != 'function') {
error(`no alias found for ${x}`);
}

const argCount = sig.length - 1;
if (argCount !== lib[target].length) {
error(`incorrect number of arguments in signature of ${x} (declared: ${argCount}, expected: ${lib[target].length})`);
}
const ret = sig == 'v' ? '' : 'return ';
const args = genArgSequence(argCount).join(',');
const mangledName = mangleCSymbolName(target);
lib[x] = new Function(args, `${ret}${mangledName}(${args});`);

if (!lib[x + '__deps']) lib[x + '__deps'] = [];
lib[x + '__deps'].push(target);
for (const ident in this.library) {
if (isJsLibraryConfigIdentifier(ident)) {
const index = ident.lastIndexOf('__');
const basename = ident.slice(0, index);
if (!(basename in this.library)) {
error(`Missing library element '${basename}' for library config '${ident}'`);
Copy link
Member

Choose a reason for hiding this comment

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

This code now only does error handling?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes this loop was handling two things, despite the comment.

This is a useful error message and we test for it so we need to keep it around.

}
}
}
Expand Down