Skip to content
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

add abstract Floating trait. todo: impl Floating for Float #1204

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bobzhang
Copy link
Contributor

@bobzhang bobzhang commented Nov 8, 2024

No description provided.

todo: impl Floating for Float
Copy link

peter-jerry-ye-code-review bot commented Nov 8, 2024

Observations from the git diff Output

  1. Trait Method Ambiguity in math/float.mbt:

    • The trait Floating in math/float.mbt defines methods asin and acos without using the pub fn asin(x : Self) -> Self syntax, which could lead to ambiguity if the trait methods are intended to be used directly or if they should be accessed via the trait itself (e.g., @math.Floating::asin(x)). It's a good practice to clarify whether these methods should be directly accessible on the type or only through the trait.
  2. Redundancy in Function Definitions:

    • The functions asin and acos are redefined in math/trigonometric.mbt for the Double type, even though they are already implemented in the Floating trait for Double. This redundancy could lead to maintenance issues if the implementations need to be updated in the future. It's advisable to rely on the trait implementations unless there are specific optimizations or differences required for the Double type.
  3. Generics in Interface Definitions:

    • The math.mbti file updates the function signatures for acos and asin to use generics constrained by the Floating trait (fn acos[T : Floating](T) -> T and fn asin[T : Floating](T) -> T). This change is positive as it promotes reusability and adherence to the trait, ensuring that any type implementing Floating can use these functions without the need for type-specific overloads. However, ensure that all relevant types are correctly implementing the Floating trait to leverage these generic functions.

Recommendations

  1. Clarify Trait Method Access:

    • Decide whether trait methods like asin and acos should be accessed directly on types or only through the trait. If they are meant to be used through the trait, consider using @math.Floating::asin(x) to avoid ambiguity.
  2. Remove Redundant Implementations:

    • If the implementations of asin and acos for Double in math/trigonometric.mbt are identical to those provided by the Floating trait, consider removing these redundant definitions to reduce maintenance overhead and potential inconsistencies.
  3. Verify Trait Implementation:

    • Ensure that all types intended to use the generic acos and asin functions are correctly implementing the Floating trait. This will ensure that the generic functions work as expected across all relevant types.

By addressing these observations, the codebase can become more maintainable, clear, and consistent, especially in how traits and generics are used to promote code reuse and type safety.

@coveralls
Copy link
Collaborator

coveralls commented Nov 8, 2024

Pull Request Test Coverage Report for Build 3816

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 15 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.007%) to 80.214%

Files with Coverage Reduction New Missed Lines %
/Users/runner/work/core/core/math/trigonometric.mbt 15 71.88%
Totals Coverage Status
Change from base Build 3812: 0.007%
Covered Lines: 4346
Relevant Lines: 5418

💛 - Coveralls

// pub fn asin(x : Self) -> Self
acos(Self) -> Self
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note we may introduce a sugar like this:

pub trait Floating::(asin, acos)

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.

2 participants