-
Notifications
You must be signed in to change notification settings - Fork 1
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
support edn any-ref; change ABI version to 0.0.8 #243
Conversation
WalkthroughThe project has undergone version upgrades and optimizations across Rust and JavaScript components. Key changes include version increments in Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lock
is excluded by!**/*.lock
Files selected for processing (8)
- Cargo.toml (2 hunks)
- package.json (1 hunks)
- src/bin/injection/mod.rs (1 hunks)
- src/builtins/meta.rs (1 hunks)
- src/calcit.rs (5 hunks)
- src/data.rs (1 hunks)
- src/data/edn.rs (2 hunks)
- src/runner.rs (1 hunks)
Files skipped from review due to trivial changes (3)
- Cargo.toml
- package.json
- src/bin/injection/mod.rs
Additional comments not posted (8)
src/data/edn.rs (2)
101-101
: Ensure the conversion logic forCalcit::AnyRef(..)
is correct.The conversion logic for
Calcit::AnyRef(..)
toEdn::AnyRef
seems straightforward. Verify thatEdn::AnyRef
correctly handles the data encapsulated byCalcit::AnyRef
.
185-185
: Ensure the conversion logic forEdn::AnyRef(..)
is correct.The conversion logic for
Edn::AnyRef(..)
toCalcit::AnyRef
seems straightforward. Verify thatCalcit::AnyRef
correctly handles the data encapsulated byEdn::AnyRef
.src/runner.rs (1)
34-35
: LGTM! Ensure all function calls handleCalcit::AnyRef(..)
correctly.The new match arm for
Calcit::AnyRef(..)
inevaluate_expr
returns the expression as is, which seems appropriate. Ensure that all function calls handleCalcit::AnyRef(..)
correctly.src/builtins/meta.rs (1)
62-62
: Add a match arm forCalcit::AnyRef(..)
.The addition of the
Calcit::AnyRef(..)
match arm in thetype_of
function is correct and consistent with the existing pattern. This ensures that the newAnyRef
variant is properly handled.src/calcit.rs (4)
100-101
: The addition of theAnyRef
variant to theCalcit
enum looks good. Ensure that all necessary implementations and match arms are updated to handle this new variant.
277-277
: Thefmt::Display
implementation forAnyRef
is correctly added. Ensure that the format string is appropriate for the intended output.
445-447
: TheHash
implementation forAnyRef
uses theunreachable!
macro, which is appropriate ifAnyRef
should not be hashed. Ensure that this case is indeed unreachable in practice.
592-592
: TheOrd
implementation forAnyRef
uses theunreachable!
macro, which is appropriate ifAnyRef
should not be compared. Ensure that this case is indeed unreachable in practice.
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.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lock
is excluded by!**/*.lock
Files selected for processing (3)
- Cargo.toml (2 hunks)
- package.json (1 hunks)
- src/calcit.rs (5 hunks)
Files skipped from review as they are similar to previous changes (3)
- Cargo.toml
- package.json
- src/calcit.rs
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.
Actionable comments posted: 9
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lock
is excluded by!**/*.lock
Files selected for processing (14)
- Cargo.toml (2 hunks)
- package.json (1 hunks)
- src/bin/cr.rs (1 hunks)
- src/builtins.rs (1 hunks)
- src/builtins/lists.rs (8 hunks)
- src/builtins/maps.rs (1 hunks)
- src/builtins/meta.rs (2 hunks)
- src/builtins/records.rs (5 hunks)
- src/builtins/refs.rs (1 hunks)
- src/builtins/syntax.rs (1 hunks)
- src/codegen/emit_js.rs (1 hunks)
- src/data.rs (1 hunks)
- src/runner.rs (2 hunks)
- src/util/string.rs (1 hunks)
Files not reviewed due to errors (1)
- src/builtins/records.rs (no review received)
Files skipped from review as they are similar to previous changes (5)
- Cargo.toml
- package.json
- src/builtins/meta.rs
- src/data.rs
- src/runner.rs
Additional comments not posted (6)
src/builtins/refs.rs (1)
29-29
: LGTM! The use ofclone_into
is appropriate here.src/builtins/maps.rs (2)
82-82
: LGTM! The use ofclone_into
is appropriate here.
86-86
: LGTM! The use ofclone_into
is appropriate here.src/bin/cr.rs (1)
107-107
: LGTM! This change ensures that the snapshot configuration is updated with the entry configuration.src/builtins.rs (1)
49-51
: LGTM! This change ensures that stack information is attached to errors from procs.src/codegen/emit_js.rs (1)
852-853
: Ensure the cloned elements are necessary.The cloning of elements into mutable variables seems correct, but please verify if this is necessary for the intended logic.
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.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lock
is excluded by!**/*.lock
Files selected for processing (2)
- Cargo.toml (4 hunks)
- package.json (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- Cargo.toml
- package.json
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.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lock
is excluded by!**/*.lock
Files selected for processing (2)
- Cargo.toml (4 hunks)
- package.json (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- Cargo.toml
- package.json
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.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lock
is excluded by!**/*.lock
Files selected for processing (2)
- Cargo.toml (4 hunks)
- package.json (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- Cargo.toml
- package.json
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.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/bin/injection/mod.rs (3 hunks)
Additional comments not posted (5)
src/bin/injection/mod.rs (5)
22-25
: Introduce caching for dynamic libraries usinglazy_static
.This change introduces a lazy initialization pattern for caching dynamic libraries, which is a good practice to avoid repeated loading and unloading of libraries, potentially improving performance and stability.
27-36
: Implementload_dylib
function to manage dynamic library loading and caching.This function effectively manages the loading of dynamic libraries and caches them to avoid redundant loads. This is crucial for performance in environments where frequent library calls are made.
38-38
: UpdateABI_VERSION
to "0.0.8".This change aligns with the PR's objective to update the ABI version, ensuring compatibility with the latest changes.
73-81
: Refactorcall_dylib_edn
to use the newload_dylib
function.This refactoring ensures that the function leverages the new caching mechanism introduced for dynamic libraries, which should improve performance and maintainability.
141-147
: Refactorcall_dylib_edn_fn
to use the newload_dylib
function and handle ABI version checks.The refactoring here is consistent with the changes made in
call_dylib_edn
, applying the same improvements and ensuring that the ABI version is checked before proceeding with function calls.
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.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lock
is excluded by!**/*.lock
Files selected for processing (2)
- Cargo.toml (4 hunks)
- package.json (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- Cargo.toml
- package.json
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.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lock
is excluded by!**/*.lock
Files selected for processing (2)
- Cargo.toml (4 hunks)
- package.json (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- Cargo.toml
- package.json
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.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lock
is excluded by!**/*.lock
Files selected for processing (4)
- Cargo.toml (4 hunks)
- package.json (2 hunks)
- src/bin/injection/mod.rs (3 hunks)
- src/util/string.rs (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- Cargo.toml
- package.json
- src/util/string.rs
Additional comments not posted (5)
src/bin/injection/mod.rs (5)
22-25
: Introduce caching for dynamic libraries usinglazy_static
.This change introduces a global cache for dynamic libraries, which can help in reducing the overhead of loading libraries multiple times. Good use of
lazy_static
for thread-safe, lazy initialization.
27-36
: Refactorload_dylib
to utilize caching mechanism.The refactoring of
load_dylib
to use the newly introduced cache is a good practice. It avoids reloading the libraries multiple times which can be resource-intensive. The use ofunwrap
is generally risky but acceptable here assuming the lock will not be poisoned.
38-38
: UpdateABI_VERSION
to "0.0.9".Updating the ABI version is crucial for maintaining compatibility with the changes in the dynamic libraries. Ensure that all dependent systems are aware of this version change.
73-81
: Ensure ABI version compatibility before invoking library functions.This is a crucial check to ensure that the ABI version of the loaded library matches the expected version. This prevents potential runtime errors due to ABI incompatibilities. Good use of error handling to provide clear feedback if versions do not match.
141-147
: Duplicate ABI version check logic incall_dylib_edn_fn
.The ABI version check logic is repeated in
call_dylib_edn_fn
. Consider abstracting this logic into a separate function to avoid code duplication and improve maintainability.
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.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lock
is excluded by!**/*.lock
Files selected for processing (4)
- Cargo.toml (4 hunks)
- src/builtins/records.rs (11 hunks)
- src/calcit/record.rs (1 hunks)
- src/call_stack.rs (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- Cargo.toml
- src/builtins/records.rs
Additional comments not posted (1)
src/call_stack.rs (1)
128-128
: Enhanced error logging indisplay_stack
by including stack kind.This change should help in better understanding the context of errors during debugging.
@@ -100,7 +100,7 @@ impl CalcitRecord { | |||
} | |||
|
|||
let new_name_id: EdnTag = match new_tag { | |||
Calcit::Str(s) | Calcit::Symbol { sym: s, .. } => EdnTag::new(s), | |||
Calcit::Str(s) | Calcit::Symbol { sym: s, .. } => EdnTag(s.to_owned()), |
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.
Ensure comprehensive error handling for new Calcit
types in extend_field
.
Consider adding specific error messages or handling for other Calcit
types that might not be covered by the current implementation.
based on Cirru/cirru-edn.rs#41