Expand :|| and :!
#47
Conversation
WalkthroughThe pull request introduces several modifications to the Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
lib/charms/prelude.ex (1)
29-33: Consider extracting duplicated type validation logicThe integer type validation logic is duplicated between
wrap_argandconstant_of_same_type. Consider extracting this into a private helper function.Here's a suggested refactor:
+ defp validate_integer_type!(t) do + if MLIR.CAPI.mlirTypeIsAInteger(t) |> Beaver.Native.to_term() do + :ok + else + raise ArgumentError, "Not an integer type, #{to_string(t)}" + end + end defp constant_of_same_type(i, v, opts) do mlir ctx: opts[:ctx], block: opts[:block] do t = MLIR.CAPI.mlirValueGetType(v) - if MLIR.CAPI.mlirTypeIsAInteger(t) |> Beaver.Native.to_term() do - Arith.constant(value: Attribute.integer(t, i)) >>> t - else - raise ArgumentError, "Not an integer type for constant, #{to_string(t)}" - end + validate_integer_type!(t) + Arith.constant(value: Attribute.integer(t, i)) >>> t end end defp wrap_arg({i, t}, opts) when is_integer(i) do mlir ctx: opts[:ctx], block: opts[:block] do case i do %MLIR.Value{} -> i i when is_integer(i) -> - if MLIR.CAPI.mlirTypeIsAInteger(t) |> Beaver.Native.to_term() do - Arith.constant(value: Attribute.integer(t, i)) >>> t - else - raise ArgumentError, "Not an integer type, #{to_string(t)}" - end + validate_integer_type!(t) + Arith.constant(value: Attribute.integer(t, i)) >>> t end end endtest/defm_test.exs (2)
105-110: Consider adding edge case testsWhile the basic functionality is tested, consider adding tests for:
- Maximum integer values to verify overflow handling
- Negative numbers for add0 (which requires positive numbers)
- Multiple error cases for add_or_error
Example test cases to add:
# Test maximum integer handling assert_raise ArgumentError, fn -> AddTwoInt.add(System.max_integer(), 1).(engine) end # Test negative number handling for add0 assert_raise ArgumentError, fn -> AddTwoInt.add0(-1, 2).(engine) end
Line range hint
146-163: Consider more specific error message patternsThe current regex patterns are quite broad. Consider making them more specific to catch exact error conditions while still maintaining flexibility.
Example of more specific pattern:
-~r"Failed to expand macro Elixir.DifferentCalls.something/1.+function something not found in module DifferentCalls" +~r"Failed to expand macro Elixir\.DifferentCalls\.something/1.*function something not found in module DifferentCalls$"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
lib/charms/defm/expander.ex(9 hunks)lib/charms/prelude.ex(3 hunks)test/defm_test.exs(5 hunks)
🔇 Additional comments (11)
lib/charms/prelude.ex (3)
13-18: LGTM! Good type safety improvement
The added type validation ensures that constants are only created for integer types, with a clear error message. This is a good defensive programming practice.
65-67: LGTM! Clean implementation of logical OR
The implementation of :|| using Arith.ori follows the established pattern of other binary operators and maintains consistency in the codebase.
8-8: LGTM, but verify :! operator implementation
The addition of :|| to @binary_ops is correct. However, the PR objectives mention expanding both :|| and :! operators, but :! is not included in the binary operators list.
Let's check if :! is implemented elsewhere:
test/defm_test.exs (1)
5-27: Consider adding memory cleanup for allocated pointers
The function allocates memory for pointers but doesn't explicitly free them. While LLVM might handle this automatically, it's worth verifying the memory management strategy.
Consider implementing a with-cleanup pattern or ensuring the runtime properly handles pointer cleanup.
lib/charms/defm/expander.ex (7)
282-284: Proper usage of uniq_mlir_var with updated parameter order
The function uniq_mlir_var is correctly called with the new parameter order (val, state), which aligns with the updated function signature.
473-473: Consistent usage of uniq_mlir_params with updated parameter order
The call to uniq_mlir_params(args, state) reflects the updated parameter order, maintaining consistency throughout the code.
695-709: Effective exception handling in prelude intrinsic expansion
The try...rescue block appropriately handles exceptions during the expansion of prelude intrinsics. The error message provides clear and informative feedback, aiding in debugging any issues that may arise.
1286-1286: Consistent usage of uniq_mlir_params with updated parameter order
The function uniq_mlir_params is correctly used with (args, state), maintaining consistency with the updated function signature.
1322-1322: Appropriate use of uniq_mlir_params in local function expansion
The call to uniq_mlir_params(args, state) in expand_local reflects the updated parameter order, ensuring consistency across the codebase.
1409-1409: Correct application of uniq_mlir_var in beam_env_from_defm!
The function uniq_mlir_var(e, state) is used correctly, adhering to the updated parameter order.
1420-1426: Overloaded uniq_mlir_var functions enhance flexibility
The introduction of an overloaded uniq_mlir_var/0 function complements the existing uniq_mlir_var/2 function. This provides flexibility in generating unique MLIR variables both with and without initial values.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
test/defm_test.exs (1)
105-110: Add test cases for boundary conditions and overflowThe current test suite should be expanded to include:
- Integer overflow scenarios
- Boundary value testing (MIN_INT, MAX_INT)
- Edge cases for add0's positive number requirement
Consider adding these test cases:
# Test integer overflow assert_raise ArgumentError, fn -> AddTwoInt.add(9223372036854775807, 1).(engine) end # Test boundary values assert AddTwoInt.add(-9223372036854775808, 0).(engine) == -9223372036854775808 assert AddTwoInt.add(9223372036854775807, 0).(engine) == 9223372036854775807 # Test add0 with zero and negative numbers assert_raise ArgumentError, fn -> AddTwoInt.add0(0, 1).(engine) end assert_raise ArgumentError, fn -> AddTwoInt.add0(-1, 1).(engine) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
test/defm_test.exs(5 hunks)
🔇 Additional comments (1)
test/defm_test.exs (1)
64-89: LGTM! Well-structured error message assertions
The use of dynamic line numbers and regex patterns makes the tests more maintainable and robust.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
test/defm_test.exs (3)
5-27: Consider refactoring error handling approachThe conditional branching approach using
cond_bris more complex than necessary. The simpler if-else pattern used in theaddfunction achieves the same result with better readability.Consider simplifying to:
- defm add_or_error_with_cond_br(env, a, b, error) :: Term.t() do - ptr_a = Pointer.allocate(i32()) - ptr_b = Pointer.allocate(i32()) - - arg_err = - block do - func.return(error) - end - - cond_br enif_get_int(env, a, ptr_a) != 0 do - cond_br 0 != enif_get_int(env, b, ptr_b) do - a = Pointer.load(i32(), ptr_a) - b = Pointer.load(i32(), ptr_b) - sum = value llvm.add(a, b) :: i32() - term = enif_make_int(env, sum) - func.return(term) - else - ^arg_err - end - else - ^arg_err - end - end + defm add_or_error_with_cond_br(env, a, b, error) :: Term.t() do + ptr_a = Pointer.allocate(i32()) + ptr_b = Pointer.allocate(i32()) + + if enif_get_int(env, a, ptr_a) != 0 && enif_get_int(env, b, ptr_b) != 0 do + a = Pointer.load(i32(), ptr_a) + b = Pointer.load(i32(), ptr_b) + sum = value llvm.add(a, b) :: i32() + enif_make_int(env, sum) + else + error + end + end
92-95: Add boundary test cases for integer operationsThe current test cases only verify basic addition functionality. Consider adding tests for:
- Edge cases (MIN_INT32, MAX_INT32)
- Overflow scenarios
- Zero value handling
Example additional test cases:
# Test maximum integer values assert AddTwoInt.add(2_147_483_647, 1).(engine) == -2_147_483_648 # INT32_MAX + 1 # Test minimum integer values assert AddTwoInt.add(-2_147_483_648, -1).(engine) == 2_147_483_647 # INT32_MIN - 1
131-132: Improve error message pattern matchingThe regex pattern for undefined function error could be more specific to avoid potential false matches.
Consider using a more precise pattern:
-~r"Failed to expand macro Elixir.DifferentCalls.something/1.+function something not found in module DifferentCalls" +~r"^Failed to expand macro Elixir\.DifferentCalls\.something/1.*function something not found in module DifferentCalls$"Also applies to: 134-134
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
test/defm_test.exs(5 hunks)
🔇 Additional comments (1)
test/defm_test.exs (1)
6-7:
Integer type inconsistency issue
Both functions use i32 which could lead to integer overflow issues. Previous review comments have flagged this inconsistency with i64 usage in other parts of the codebase.
Also applies to: 30-31
:||and:!i1inifSummary by CodeRabbit
New Features
:||in binary operations.add_or_error_with_cond_brandadd.expand_macrofunction for theKernel.!operator.Bug Fixes
Documentation