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

use golang built-in functions rather than mathutil #56594

Closed
lance6716 opened this issue Oct 12, 2024 · 18 comments · Fixed by #56818 · May be fixed by #56620 or #56621
Closed

use golang built-in functions rather than mathutil #56594

lance6716 opened this issue Oct 12, 2024 · 18 comments · Fixed by #56818 · May be fixed by #56620 or #56621
Assignees
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. hacktoberfest type/enhancement The issue or PR belongs to an enhancement.

Comments

@lance6716
Copy link
Contributor

Enhancement

like we can use max rather than

// Max returns the largest one from its arguments.
func Max[T constraints.Ordered](x T, xs ...T) T {
maxv := x
for _, n := range xs {
if n > maxv {
maxv = n
}
}
return maxv
}

@lance6716 lance6716 added type/enhancement The issue or PR belongs to an enhancement. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. labels Oct 12, 2024
@Anudhyan
Copy link

I have worked on this issue @lance6716

@lance6716
Copy link
Contributor Author

/assign @Anudhyan

@Anudhyan
Copy link

Thanks a lot @lance6716

@Anudhyan
Copy link

@lance6716 when will this be reviewed?

@lance6716
Copy link
Contributor Author

@lance6716 when will this be reviewed?

I'll take a look at Monday

@Anudhyan Anudhyan mentioned this issue Oct 14, 2024
11 tasks
@Anudhyan
Copy link

@lance6716 I have updated the code using inbuilt function and it is running without errors. Please check it

@lance6716
Copy link
Contributor Author

@Anudhyan Please read the issue. Use golang built-in functions like https://pkg.go.dev/builtin#max. And delete mathutil. Max

@Anudhyan
Copy link

Anudhyan commented Oct 14, 2024

@Anudhyan Please read the issue. Use golang built-in functions like https://pkg.go.dev/builtin#max. And delete mathutil. Max

@lance6716 Do I need to do the same for this file only? [tidb/pkg/util/mathutil/math.go]

I have used slices replace custom Max Min functions and removed mathutil.max from line 45 with ^uint64(0). Please guide me if there is any error from my side. I am contributing for the first time!

@lance6716
Copy link
Contributor Author

Do I need to do the same for this file only? [tidb/pkg/util/mathutil/math.go]

No. Delete mathutil.Max and let caller use golang built-in max. Also for min and other proper functions.

@Anudhyan
Copy link

@lance6716 I was working on this issue. This is the first time I am working on issue for TiDB. Please reassign it to me

@VersatileVats
Copy link

@lance6716 are you talking about this change:

result := math.Max(float64(a), float64(b))

@lance6716
Copy link
Contributor Author

@lance6716 are you talking about this change:

result := math.Max(float64(a), float64(b))

I don't know where's the code you refer to. I think the issue is clear and I don't know how to provide more details

@VersatileVats
Copy link

VersatileVats commented Oct 22, 2024

Okay, thanks for the clarification! So, @Anudhyan you have to delete mathutil.Max and mathutil.Min and replace them with Go's built-in max and min functions

@lance6716
Copy link
Contributor Author

Okay, thanks for the clarification! So, @Anudhyan you have to delete mathutil.Max and mathutil.Min and replace them with Go's built-in math.Max and math.Min functions

The built-in functions are called max, min rather than math.Max and math.Min

@Anudhyan
Copy link

Okay, thanks for the clarification! So, @Anudhyan you have to delete mathutil.Max and mathutil.Min and replace them with Go's built-in math.Max and math.Min functions

The built-in functions are called max, min rather than math.Max and math.Min

@lance6716 the built in max and min are not variadic in nature. So how will I implement it here for variable n set of arguments?

@lance6716
Copy link
Contributor Author

Obviously max and min ARE variadic in nature. Please learn some basic golang knowledge before you contribute.

@lance6716
Copy link
Contributor Author

/assign @songzhibin97

Copy link

ti-chi-bot bot commented Oct 24, 2024

@lance6716: GitHub didn't allow me to assign the following users: songzhibin97.

Note that only pingcap members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @songzhibin97

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. hacktoberfest type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
3 participants