-
Notifications
You must be signed in to change notification settings - Fork 5
Small changes for correctness #372
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
Changes from all commits
60e6da2
4b1b76d
d71c325
00660a4
8d42701
8eaf9fd
f06fc9f
0ec6083
4137463
94d7e19
368455d
e29d393
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,7 +73,7 @@ public static bool Near( | |
| var rel = relTol ?? 1e-9; | ||
| var abs = absTol ?? 0.0; | ||
| var margin = Math.Max(Math.Max(Math.Abs(x), Math.Abs(y)) * rel, abs); | ||
| return Math.Abs(x - y) < margin; | ||
| return Math.Abs(x - y) <= margin; | ||
| } | ||
|
|
||
| public static double Sign(this double x) | ||
|
|
@@ -109,7 +109,7 @@ public static double ToFloat64(this string s) | |
| ? double.PositiveInfinity | ||
| : trimmed == "-Infinity" | ||
| ? double.NegativeInfinity | ||
| : double.Parse(s); | ||
| : double.Parse(trimmed); | ||
|
Contributor
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. In Remarks here, it says that |
||
| } | ||
|
|
||
| public static int ToInt(double value) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,7 +62,7 @@ public static int CountBetween(string s, int left, int right) | |
| for (int i = Math.Max(0, left); i < limit; ++i) | ||
| { | ||
| count += 1; | ||
| if (i + 1 < right) | ||
| if (i + 1 < limit) | ||
|
Contributor
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. Because the loop prevents |
||
| { | ||
| char c = s[i]; | ||
| if ('\uD800' <= c && c <= '\uDBFF') | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ export const float64Near = (x, y, relTol, absTol) => { | |
| absTol = 0; | ||
| } | ||
| const margin = Math.max(Math.max(Math.abs(x), Math.abs(y)) * relTol, absTol); | ||
| return Math.abs(x - y) < margin; | ||
| return Math.abs(x - y) <= margin; | ||
|
Contributor
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. So we've updated cs and js so far. |
||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -238,7 +238,7 @@ export const listBuilderReverse = (ls) => { | |
| * @param {T} newValue | ||
| */ | ||
| export const listBuilderSet = (ls, i, newValue) => { | ||
| if (0 <= i && i <= ls.length) { | ||
| if (0 <= i && i < ls.length) { | ||
| ls[i] = newValue; | ||
| } | ||
|
Contributor
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. I guess the old version allowed for expanding the array on
So I agree this is a good fix. |
||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -159,7 +159,7 @@ export const stringHasAtLeast = (s, begin, end, minCount) => { | |
| * @returns {number} | ||
| */ | ||
| export const stringNext = (s, i) => { | ||
| let iNext = Math.min(s.length, i); | ||
| let iNext = Math.min(s.length, Math.max(0, i)); | ||
|
Contributor
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. Because this is our spec: public next(index: StringIndex): StringIndex;It's illegal to pass in negative numbers, and we currently have no specific behavior for illegal negative values. Old behavior would return the same invalid index passed in. New behavior treats it as index 0. I think the old behavior is better, but maybe a comment would be good. And maybe better yet would be an exception on bad value, but that might slow down the common case of valid parameter values. Overall, maybe add a comment, but I'd like to revert this change. |
||
| let cp = s.codePointAt(i); | ||
| if (cp !== undefined) { | ||
| iNext += 1 + !!(cp >>> 16); | ||
|
|
@@ -173,7 +173,7 @@ export const stringNext = (s, i) => { | |
| * @returns {number} | ||
| */ | ||
| export const stringPrev = (s, i) => { | ||
| let iPrev = Math.min(s.length, i); | ||
| let iPrev = Math.min(s.length, Math.max(0, i)); | ||
|
Contributor
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. I haven't reviewed carefully here, but I suspect the old behavior is better again, as for stringNext. |
||
| if (iPrev) { | ||
| iPrev -= 1; | ||
| if (iPrev && s.codePointAt(iPrev - 1) >>> 16) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,25 +3,32 @@ package lang.temper.be.lua | |
| // Could go up to 200 in theory, but smaller numbers allow for larger closures. | ||
| const val MAX_ALLOWABLE_LOCALS = 128 | ||
|
|
||
| // When wrapping is needed, keep first N locals as real locals for performance. | ||
| // The rest overflow into the env table. Budget: LOCALS_TO_KEEP + 1 (env table) < MAX_ALLOWABLE_LOCALS. | ||
| private const val LOCALS_TO_KEEP = 100 | ||
|
|
||
| class LuaLocalRemover { | ||
| private var locals = mutableMapOf<LuaName, LuaName>() | ||
| private var luaName = LuaName("_G") | ||
| private var numLocalTables = 0 | ||
| private var shouldRewrite = false | ||
| private var needsEnvTable = false | ||
| private var localsKept = 0 | ||
| private var localsToDecl = mutableListOf<LuaName>() | ||
|
|
||
| private fun allocLocalTableName(): LuaName = LuaName("env_t${++numLocalTables}") | ||
|
|
||
| private fun scoped(cb: () -> Lua.Chunk): Lua.Chunk { | ||
| val lastLocals = locals.toMutableMap() | ||
| val lastLuaName = luaName | ||
| val lastShouldRewrite = shouldRewrite | ||
| val lastNeedsEnvTable = needsEnvTable | ||
| val lastLocalsKept = localsKept | ||
| val lastLocalsToDecl = localsToDecl | ||
| localsToDecl = mutableListOf() | ||
| val ret = cb() | ||
| locals = lastLocals | ||
| luaName = lastLuaName | ||
| shouldRewrite = lastShouldRewrite | ||
| needsEnvTable = lastNeedsEnvTable | ||
| localsKept = lastLocalsKept | ||
| val localsToWrap = localsToDecl | ||
| localsToDecl = lastLocalsToDecl | ||
| return luaChunk( | ||
|
|
@@ -50,12 +57,12 @@ class LuaLocalRemover { | |
| ) | ||
| } | ||
|
|
||
| private fun define(target: Lua.SetTarget) { | ||
| private fun define(target: Lua.SetTarget, asEnvField: Boolean) { | ||
| when (target) { | ||
| is Lua.DotSetTarget -> TODO() | ||
| is Lua.IndexSetTarget -> TODO() | ||
| is Lua.NameSetTarget -> { | ||
| if (shouldRewrite) { | ||
| if (asEnvField) { | ||
| locals[target.target.id] = luaName | ||
| } else { | ||
| locals.remove(target.target.id) | ||
|
|
@@ -64,6 +71,10 @@ class LuaLocalRemover { | |
| } | ||
| } | ||
|
|
||
| // Decide whether a batch of N new locals should overflow to the env table. | ||
| private fun shouldOverflow(count: Int): Boolean = | ||
| needsEnvTable && localsKept + count > LOCALS_TO_KEEP | ||
|
|
||
| private fun set(target: Lua.SetTarget): Lua.SetTarget = when (target) { | ||
| is Lua.DotSetTarget -> when (val obj = target.obj) { | ||
| is Lua.SetTarget -> Lua.DotSetTarget( | ||
|
|
@@ -230,8 +241,9 @@ class LuaLocalRemover { | |
| private fun wrapLocals(chunk: Lua.Chunk): List<Lua.Stmt> { | ||
| luaName = allocLocalTableName() | ||
| val numLocals = countLocalsDirectlyIn(chunk) | ||
| shouldRewrite = numLocals >= MAX_ALLOWABLE_LOCALS | ||
| if (shouldRewrite) { | ||
| needsEnvTable = numLocals >= MAX_ALLOWABLE_LOCALS | ||
| if (needsEnvTable) { | ||
| localsKept = 0 | ||
| return listOf( | ||
| Lua.LocalStmt( | ||
| chunk.pos, | ||
|
|
@@ -355,17 +367,17 @@ class LuaLocalRemover { | |
| Lua.Name(stmt.name.pos, stmt.name.id), | ||
| ) | ||
| is Lua.LocalDeclStmt -> { | ||
| stmt.targets.targets.forEach(::define) | ||
| if (shouldRewrite) { | ||
| null | ||
| } else { | ||
| val overflow = shouldOverflow(stmt.targets.targets.size) | ||
| stmt.targets.targets.forEach { define(it, asEnvField = overflow) } | ||
| if (!overflow) { | ||
| if (needsEnvTable) localsKept += stmt.targets.targets.size | ||
| stmt.targets.targets.forEach { | ||
| localsToDecl.add((it as Lua.NameSetTarget).target.id) | ||
| } | ||
| null | ||
| } | ||
| null | ||
| } | ||
| is Lua.LocalFunctionStmt -> if (shouldRewrite) { | ||
| is Lua.LocalFunctionStmt -> if (shouldOverflow(1)) { | ||
| locals[stmt.name.id] = luaName | ||
| Lua.SetStmt( | ||
| stmt.pos, | ||
|
|
@@ -405,6 +417,7 @@ class LuaLocalRemover { | |
| ), | ||
| ) | ||
| } else { | ||
| if (needsEnvTable) localsKept++ | ||
| localsToDecl.add(stmt.name.id) | ||
| Lua.SetStmt( | ||
| stmt.pos, | ||
|
|
@@ -440,7 +453,7 @@ class LuaLocalRemover { | |
| ), | ||
| ) | ||
| } | ||
| is Lua.LocalStmt -> if (shouldRewrite) { | ||
| is Lua.LocalStmt -> if (shouldOverflow(stmt.targets.targets.size)) { | ||
| val ret = Lua.SetStmt( | ||
| stmt.pos, | ||
| Lua.SetTargets( | ||
|
|
@@ -461,9 +474,10 @@ class LuaLocalRemover { | |
| stmt.exprs.exprs.map(::scan), | ||
| ), | ||
| ) | ||
| stmt.targets.targets.forEach(::define) | ||
| stmt.targets.targets.forEach { define(it, asEnvField = true) } | ||
| ret | ||
| } else { | ||
| if (needsEnvTable) localsKept += stmt.targets.targets.size | ||
| val ret = Lua.SetStmt( | ||
| stmt.pos, | ||
| Lua.SetTargets( | ||
|
|
@@ -479,7 +493,7 @@ class LuaLocalRemover { | |
| stmt.exprs.exprs.map(::scan), | ||
| ), | ||
| ) | ||
| stmt.targets.targets.forEach(::define) | ||
| stmt.targets.targets.forEach { define(it, asEnvField = false) } | ||
|
Contributor
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. I can't easily just review this by looking through code. I'll rely on whatever analysis and testing you've done. Could you report that here?
Contributor
Author
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. It literally just sets |
||
| ret | ||
| } | ||
| is Lua.SetStmt -> { | ||
|
|
||
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.
This does match Python's
math.isclosebetter. I'll plan to see if everything else also gets updated in this pr.