-
Notifications
You must be signed in to change notification settings - Fork 0
Add a scenario for performance benchmarks #1754
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
base: main
Are you sure you want to change the base?
Conversation
ae0fbe1 to
9c83256
Compare
|
@codex review |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for i := range nbInstances { | ||
| o := patchObject(obj, func(s string) string { | ||
| return strings.ReplaceAll(s, "{{i}}", strconv.Itoa(i)) |
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.
Replace invalid range loop over integer
The churn job builds with for i := range nbInstances where nbInstances is an int. Primitive integers are not iterable, so this code does not compile (cannot range over nbInstances (type int)). The same construct appears in the deferred cleanup loop as well. Iterate with an index (for i := 0; i < nbInstances; i++) or convert to a slice to allow the project to build.
Useful? React with 👍 / 👎.
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.
@codex You should read https://go.dev/doc/go1.22#language.
It seems that your Go knowledge is a little bit outdated.
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.
To use Codex here, create an environment for this repo.
| baseName := baseName + "-" + platform | ||
|
|
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.
Fix short variable declaration with no new identifiers
After adding the baseName parameter, the Linux path still redeclares it with baseName := baseName + "-" + platform. Because no new identifiers appear on the left side, this statement fails to compile (no new variables on left side of :=). Use an assignment (baseName = baseName + "-" + platform) or a new variable name so the function builds.
Useful? React with 👍 / 👎.
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.
@codex It actually does compile.
60d7d2d to
7085821
Compare
7085821 to
b5e3b2a
Compare
What does this PR do?
Which scenarios this will impact?
Motivation
Additional Notes