XS code generators, object groups, etc #431
Conversation
|
This replaced PR #428. It attempts to address all feedback. It adds generators for Base64 and Hex. |
| b.unary(.PostInc, index) | ||
| let result = b.callFunction(gc, withArgs: [index]) | ||
| b.buildIf(result) { | ||
| b.loopBreak(); |
There was a problem hiding this comment.
nit: maybe remove the semicolon here.
There was a problem hiding this comment.
(Removed all semicolons from the Swift code. It took me forever to learn to add semicolons to C code.... so the habit of adding them is now tough to break!)
| } | ||
|
|
||
| fileprivate let ModuleSourceGenerator = RecursiveCodeGenerator("ModuleSourceGenerator") { b in | ||
| let moduleSourceConstructor = b.loadBuiltin("ModuleSource"); |
There was a problem hiding this comment.
nit: drop the semicolon
| } | ||
|
|
||
| fileprivate let CompartmentGenerator = RecursiveCodeGenerator("CompartmentGenerator") { b in | ||
| let compartmentConstructor = b.loadBuiltin("Compartment"); |
| b.buildRecursive(block: 3, of: 4) | ||
| b.doReturn(b.randomVariable()) | ||
| } | ||
| options["resolveHook"] = resolveHook; |
There was a problem hiding this comment.
nit: drop the semicolons
| var s = "" | ||
| for _ in 0..<Int.random(in: 1...100) { | ||
| let codePoint = UInt32.random(in: 0..<0x10FFFF) | ||
| if ((0xD800 <= codePoint) && (codePoint < 0xE000)) { |
There was a problem hiding this comment.
Can we rewrite this to something like this:
// Ignore surrogate pair code points
if !((0xD800 <= codePoint) && (codePoint < 0xE000)) {
s += String(Unicode.Scalar(codePoint)!)
}
?
There was a problem hiding this comment.
Sure, will do that. FWIW – for me, it is more readable as originally written but I understand that it is generally frowned upon to have empty code blocks like that.
|
|
||
| if probability(0.5) { | ||
| b.callMethod("fromHex", on: Uint8Array, withArgs: [hex]) | ||
| } |
There was a problem hiding this comment.
nit: could you join these two lines?
e.g. } else {?
| s += chooseUniform(from: alphabet) | ||
| } | ||
|
|
||
| if probability(0.5) { |
There was a problem hiding this comment.
nit: maybe add a small comment explaining what you're doing here, e.g.
// Pad the base64 to make it valid.
Should this really be modulo 4 here? And is the intention with probability(0.5) to have some invalid Base64 sometimes? If so, do you really want to stress the error path here in half of your generations? If this throws, you will greatly reduce your correctness rate.
a=== is not valid base64 whereas aa== is, I think the logic to pad needs a bit more work?
There was a problem hiding this comment.
Yeah, this code is naive. Patching up an arbitrary length Base64 string is tricky. I'm experimenting with instead just generating a valid tail on a run of Base64 bytes that is a multiple of 4. That seems better.
The generator can always create valid inputs. The fuzzer will generate plenty of invalid inputs from there.
| let optionsObject = b.createObject(with: options) | ||
| if probability(0.5) { | ||
| b.callMethod("fromBase64", on: Uint8Array, withArgs: [base64, optionsObject]) | ||
| } |
There was a problem hiding this comment.
nit: also please join these two lines
| b.doReturn(resultVar) | ||
| } | ||
|
|
||
| // b.eval("%SetForceSlowPath(false)"); |
There was a problem hiding this comment.
I think we can just remove the comments.
And I also don't know if we then need three calls to the function or if one will suffice, that depends on XS but one should be enough? Unless you also JIT compile your regular expressions.
There was a problem hiding this comment.
XS does not have a JIT. FWIW – I commented out the Iines rather than removing them to keep the original as intact as possible for future updates. But, I can see where that can be confusing. I'll remove those.
|
I've pushed changes that should address the remaining comments. Fingers crossed. |
| } | ||
|
|
||
| // extend by 0, 1, or 2 bytes | ||
| switch (Int.random(in: 0...3)) { |
There was a problem hiding this comment.
Hm alternatively how about this idea:
We generate a random byte string, then Base64 encode that in swift and then load that as a string? :)
Then we can drop the variables and all of this?
var randomDataLength = UInt8.random(in: UInt8.min...UInt8.max)
var randomData = Data(capacity: randomDataLength)
for i in 0..<randomDataLength {
randomData[i] = Uint8.random(in: UInt8.min...Uint8.max)
}
let base64 = b.loadString(randomData.base64EncodedString())
WDYT?
https://developer.apple.com/documentation/foundation/data/2142853-base64encodedstring
There was a problem hiding this comment.
Nice idea. However, it would eliminate the tests for the "base64url" alphabet (Swift's Base64 encoder doesn't seem to have that option). I think I'd like to keep the Base64 generation: it isn't that much code and it makes it straightforward to add other options later like whitespace and invalid inputs.
|
I think the latest PR addresses all the essential requests. What do you think about merging this? |
No description provided.