Update DecodedResultsTsType to use bigint in Emscripten binding (fixes #33).
#34
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)src/clp_ffi_js/ir/StreamReader.cpp (2)
The change from
While the type change is correct, we should ensure all JavaScript/TypeScript code consuming the ✅ Verification successfulType change appears to be an internal implementation detail Based on the investigation:
Since this is a compiled FFI library where the type change is handled at the binding layer and properly registered, there's no need for manual verification of JavaScript consumers. The TypeScript type definitions will be generated during the build process to match the new type signature. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for usages of decodeRange to identify potential consumers that need updating
rg -l "decodeRange" --type ts --type js
# Search for any existing bigint type usage to verify consistency
rg -l "bigint" --type ts --type js
Length of output: 372 Script: #!/bin/bash
# Let's first find all TypeScript and JavaScript files in the repository
fd "\.(js|ts|jsx|tsx)$"
# Then search for decodeRange usage in all files
rg "decodeRange" -A 3 -B 3
# Also check for any bigint usage
rg "bigint" -A 3 -B 3
Length of output: 1311 Script: #!/bin/bash
# Let's check the implementation of decode_range to understand its usage
ast-grep --pattern 'decode_range($$$) {
$$$
}'
# Let's also check for any test files that might be using this functionality
fd "test|spec" "\.(js|ts|jsx|tsx|cpp)$"
# And check the package.json or similar build files for any TypeScript configurations
fd "package.json|tsconfig.json"
Length of output: 249 Script: #!/bin/bash
# Let's check the package.json for dependencies and TypeScript configurations
cat package.json
# Let's try finding test files with a corrected syntax
fd -e js -e ts -e jsx -e tsx -e cpp . | grep -i "test\|spec"
# Let's also check if there are any direct imports of StreamReader
rg -B 3 -A 3 "import.*StreamReader|require.*StreamReader"
Length of output: 1040 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
junhaoliao
changed the title
DecodedResultsTsType.DecodedResultsTsType to use bigint in Emscripten binding.
| @@ -124,7 +124,7 @@ EMSCRIPTEN_BINDINGS(ClpStreamReader) { | |||
| .value("STRUCTURED", clp_ffi_js::ir::StreamType::Structured) | |||
| .value("UNSTRUCTURED", clp_ffi_js::ir::StreamType::Unstructured); | |||
| emscripten::register_type<clp_ffi_js::ir::DecodedResultsTsType>( | |||
| "Array<[string, number, number, number]>" | |||
| "Array<[string, bigint, number, number]>" | |||
There was a problem hiding this comment.
Note the behaviour with logEventNum of type size_t is different: #33 (comment)
There was a problem hiding this comment.
Should we change (perhaps in another PR) the log event indices into uint32_t? I feel like having more than 4B log events in a file is unlikely.
There was a problem hiding this comment.
Should we change (perhaps in another PR) the log event indices into
uint32_t?
Let's first investigate the behaviour after setting -sMEMORY64=1. If the underlying type of size_t changes to 64 bit unsigned, I think it makes sense to always use uint32_t.
junhaoliao
changed the title
DecodedResultsTsType to use bigint in Emscripten binding.DecodedResultsTsType to use bigint in Emscripten binding (fixes #33).
|
Before we merge this we should finalize what to do with json bigint, as this could possible break log viewer |
The returned timestamp is not used in any |
Yes you are correct as discussed. Nevermind for json parse as it just reduces precision |
Description
DecodedResultsTsType.Validation performed
taskto generate the compiled assets.build/clp-ffi-js/ClpFfiJs-node.d.tsandbuild/clp-ffi-js/ClpFfiJs-woker.d.tsthat the return result ofClpStreamReader.decodeRangehas been updated asArray<[string, bigint, number, number]>Summary by CodeRabbit
New Features
Bug Fixes
Chores