simplifying the candid and method meta pointer retrieval #3510
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies the candid and method meta pointer retrieval by introducing a structured approach to return both a pointer and its length, replacing the previous null-terminated string approach.
- Introduces a
PointerAndLengthstruct to bundle pointer and length data - Updates the retrieval logic to use length-based string reading instead of null-termination
- Adds error handling for missing canister names in build commands
Reviewed Changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/stable/build/commands/build/wasm_binary/rust/stable_canister_template/src/candid_and_method_meta.rs | Adds PointerAndLength struct and updates functions to return pointer with length |
| src/stable/build/commands/build/candid_and_method_meta/execute.ts | Updates string retrieval to use length-based reading with new readPointerAndLength function |
| src/experimental/build/commands/build/wasm_binary/rust/experimental_canister_template/src/candid.rs | Mirrors the stable template changes with PointerAndLength struct |
| src/build.ts | Adds validation to ensure canister name is provided for certain commands |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| function readPointerAndLength( | ||
| memoryBuffer: ArrayBuffer, | ||
| baseAddress: number | ||
| ): { pointer: number; length: number } { | ||
| const view = new DataView(memoryBuffer, baseAddress, 8); |
There was a problem hiding this comment.
The magic number 8 should be replaced with a named constant or calculated from the struct size. This represents the size of the PointerAndLength struct (4 bytes for pointer + 4 bytes for length on 32-bit systems).
| function readPointerAndLength( | |
| memoryBuffer: ArrayBuffer, | |
| baseAddress: number | |
| ): { pointer: number; length: number } { | |
| const view = new DataView(memoryBuffer, baseAddress, 8); | |
| const POINTER_AND_LENGTH_SIZE = 8; | |
| function readPointerAndLength( | |
| memoryBuffer: ArrayBuffer, | |
| baseAddress: number | |
| ): { pointer: number; length: number } { | |
| const view = new DataView(memoryBuffer, baseAddress, POINTER_AND_LENGTH_SIZE); |
There was a problem hiding this comment.
Did I do a good job resolving this?
| pointer: view.getUint32(0, true), | ||
| length: view.getUint32(4, true) |
There was a problem hiding this comment.
The offset value 4 should be a named constant representing the pointer field size. Consider defining constants for field offsets to improve maintainability.
There was a problem hiding this comment.
Did I do a good job resolving this?
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting |
1 similar comment
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting |
Contributor
feat:prefix used if functionality should be included in theFeaturessection of the release notesReviewer
feat:prefix used if functionality should be included in theFeaturessection of the release notesFeatures
Breaking Changes