-
Notifications
You must be signed in to change notification settings - Fork 230
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
Miscellaneous fixes #294
Merged
Merged
Miscellaneous fixes #294
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
30476f9
Update to actions/checkout v4
Pat-Lafon 175dd76
Bril2json off by one error for initial row
Pat-Lafon f8e213f
Stricter cli arg parsing in brili
Pat-Lafon db8ea47
Fix mod_inv.bril
Pat-Lafon 0596887
Give the ts compiler special handling for returns in then branches
Pat-Lafon 00872a7
Fix benchmarks with dead jumps
Pat-Lafon 1a276e5
Move main typecheck to typechecking rather than runtime
Pat-Lafon 67ddbe2
generalize to getLastInstr
Pat-Lafon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -764,7 +764,7 @@ function evalInstr(instr: bril.Instruction, state: State): Action { | |
let i = getInt(instr, state.env, 0); | ||
if (i > 1114111 || i < 0 || (55295 < i && i < 57344)) { | ||
throw error(`value ${i} cannot be converted to char`); | ||
} | ||
} | ||
let val = String.fromCodePoint(Number(i)); | ||
state.env.set(instr.dest, val); | ||
return NEXT; | ||
|
@@ -873,7 +873,12 @@ function parseBool(s: string): boolean { | |
|
||
function parseNumber(s: string): number { | ||
let f = parseFloat(s); | ||
if (!isNaN(f)) { | ||
// parseFloat and Number have subtly different behaviors for parsing strings | ||
// parseFloat ignores all random garbage after any valid number | ||
// Number accepts empty/whitespace only strings and rejects numbers with seperators | ||
// Use both and only accept the intersection of the results? | ||
let f2 = Number(s); | ||
if (!isNaN(f) && f === f2) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Weird but true! It'll do. |
||
return f; | ||
} else { | ||
throw error(`float argument to main must not be 'NaN'; got ${s}`); | ||
|
@@ -891,7 +896,8 @@ function parseMainArguments(expected: bril.Argument[], args: string[]) : Env { | |
let type = expected[i].type; | ||
switch (type) { | ||
case "int": | ||
let n: bigint = BigInt(parseInt(args[i])); | ||
// https://dev.to/darkmavis1980/you-should-stop-using-parseint-nbf | ||
let n: bigint = BigInt(Number(args[i])); | ||
newEnv.set(expected[i].name, n as Value); | ||
break; | ||
case "float": | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
@main { | ||
v0: int = const 1; | ||
call @erronious v0; | ||
v1: int = const 0; | ||
} | ||
@erronious(x: int): int { | ||
v1: bool = const true; | ||
br v1 .then.0 .else.0; | ||
.then.0: | ||
v2: int = id x; | ||
ret v2; | ||
.else.0: | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
erronious(1n); | ||
|
||
// This snippet checks that the ts compiler doesn't try to create an unreachable else branch | ||
function erronious(x: bigint): bigint { | ||
if (true) { | ||
return x; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -145,20 +145,20 @@ function emitBril(prog: ts.Node, checker: ts.TypeChecker): bril.Program { | |
|
||
// Check if effect statement, i.e., a call that is not a subexpression | ||
if (call.parent.kind === ts.SyntaxKind.ExpressionStatement) { | ||
builder.buildCall(callText, | ||
builder.buildCall(callText, | ||
values.map(v => v.dest)); | ||
return builder.buildInt(0); // Expressions must produce values | ||
} else { | ||
let decl = call.parent as ts.VariableDeclaration; | ||
let type = brilType(decl, checker); | ||
let name = (decl.name != undefined) ? decl.name.getText() : undefined; | ||
return builder.buildCall( | ||
callText, | ||
values.map(v => v.dest), | ||
type, | ||
callText, | ||
values.map(v => v.dest), | ||
type, | ||
name, | ||
); | ||
} | ||
} | ||
} | ||
default: | ||
throw `unsupported expression kind: ${expr.getText()}`; | ||
|
@@ -215,12 +215,18 @@ function emitBril(prog: ts.Node, checker: ts.TypeChecker): bril.Program { | |
// Statement chunks. | ||
builder.buildLabel(thenLab); | ||
emit(if_.thenStatement); | ||
builder.buildEffect("jmp", [], undefined, [endLab]); | ||
const then_branch_terminated = builder.isLastEmittedEffectOp("ret"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A little tricky, but makes sense! |
||
if (!then_branch_terminated) { | ||
builder.buildEffect("jmp", [], undefined, [endLab]); | ||
} | ||
builder.buildLabel(elseLab); | ||
if (if_.elseStatement) { | ||
emit(if_.elseStatement); | ||
} | ||
builder.buildLabel(endLab); | ||
// The else branch otherwise just falls through without needing a target label | ||
if (!then_branch_terminated) { | ||
builder.buildLabel(endLab); | ||
} | ||
|
||
break; | ||
} | ||
|
@@ -258,7 +264,7 @@ function emitBril(prog: ts.Node, checker: ts.TypeChecker): bril.Program { | |
break; | ||
} | ||
|
||
case ts.SyntaxKind.FunctionDeclaration: | ||
case ts.SyntaxKind.FunctionDeclaration: | ||
let funcDef = node as ts.FunctionDeclaration; | ||
if (funcDef.name === undefined) { | ||
throw `no anonymous functions!`; | ||
|
@@ -298,7 +304,7 @@ function emitBril(prog: ts.Node, checker: ts.TypeChecker): bril.Program { | |
|
||
case ts.SyntaxKind.ImportDeclaration: | ||
break; | ||
|
||
default: | ||
throw `unhandled TypeScript AST node kind ${ts.SyntaxKind[node.kind]}`; | ||
} | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like useful behavior! But the name is a little odd, in two ways:
Emitted
: Maybe it would be a little more straightforward to just say it's the last op currently in the function? It seems less material that it's the last thing we emitted.Effect
: I think this would look for both effect and value ops, right?Another alternative, FWIW, would be to just get the last (non-label) instruction. So the return type would be
bril.Instruction?
, and callers could get this behavior by doing stuff likelastInstr()?.op === "add"
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, works for me.
I'll just note that I don't think the the
?
sugar works on return types? microsoft/TypeScript#51332 Maybe I missed something obvious.Instead I'm using
bril.Instruction | undefined
which the rest of the code base seems to do.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops; you're absolutely right. I forgot that
T?
was a special argument thing rather than an actual type.