Skip to content

Conversation

Yu-zh
Copy link
Collaborator

@Yu-zh Yu-zh commented Aug 1, 2025

No description provided.

@Yu-zh Yu-zh force-pushed the bleeding/migrate-core branch from 172a3a8 to c810205 Compare August 4, 2025 06:02
Copy link

peter-jerry-ye-code-review bot commented Aug 4, 2025

Inconsistent parameter conversion may break API compatibility

Category
Correctness
Code Snippet
Lines 17-27 in builtin/builtin.mbti:
-fn[A] Array::blit_to(Self[A], Self[A], len~ : Int, src_offset~ : Int = .., dst_offset~ : Int = ..) -> Unit
+fn[A] Array::blit_to(Self[A], Self[A], len~ : Int, src_offset? : Int, dst_offset? : Int) -> Unit
Recommendation
Ensure all related function calls are updated to use optional parameter syntax, or provide migration documentation. Consider keeping len~ as a named parameter for consistency since it's not made optional.
Reasoning
Converting named parameters with defaults to optional parameters changes the calling convention and may break existing code that relies on the named parameter syntax

Mixed parameter styles within the same function signature

Category
Maintainability
Code Snippet
Line 170 in builtin/builtin.mbti:
fn Hasher::new(seed? : Int) -> Self

vs

Line 19 in builtin/builtin.mbti:
fn inspect(&Show, content? : String, loc~ : SourceLoc = _, args_loc~ : ArgsLoc = _) -> Unit raise InspectError
Recommendation
Establish consistent parameter style guidelines. Either convert all parameters to optional style or maintain named parameters where they provide semantic clarity (like loc~ and args_loc~).
Reasoning
Mixing parameter styles reduces code readability and creates confusion about which style should be used in new code

Type vs struct conversion may affect binary compatibility

Category
Correctness
Code Snippet
Lines in multiple files:
-type ArgsLoc Array[SourceLoc?]
+struct ArgsLoc(Array[SourceLoc?])

-type Iter[T] ((T) -> IterResult) -> IterResult
+struct Iter[T](((T) -> IterResult) -> IterResult)
Recommendation
Verify that converting type aliases to structs doesn't break existing serialization, FFI boundaries, or pattern matching code. Consider phased migration if needed.
Reasoning
Converting type aliases to structs changes the underlying representation and may break code that depends on the previous type structure, especially in serialization or interop scenarios

@Guest0x0 Guest0x0 force-pushed the bleeding/migrate-core branch from c810205 to 53720c1 Compare August 5, 2025 10:17
@coveralls
Copy link
Collaborator

coveralls commented Aug 5, 2025

Pull Request Test Coverage Report for Build 648

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 90.081%

Totals Coverage Status
Change from base Build 641: 0.0%
Covered Lines: 8482
Relevant Lines: 9416

💛 - Coveralls

@Guest0x0 Guest0x0 merged commit 36d0db6 into main Aug 5, 2025
16 checks passed
@Guest0x0 Guest0x0 deleted the bleeding/migrate-core branch August 5, 2025 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants