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

fix: remove no needed new pointer creation #2739

Merged
merged 2 commits into from
Apr 11, 2024
Merged

Conversation

Xunzhuo
Copy link
Member

@Xunzhuo Xunzhuo commented Mar 1, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

remove no needed new pointer creation

@Xunzhuo Xunzhuo requested a review from a team as a code owner March 1, 2024 08:33
Copy link

codecov bot commented Mar 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.45%. Comparing base (c2ad8c1) to head (5bb431b).
Report is 169 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2739      +/-   ##
==========================================
- Coverage   63.48%   63.45%   -0.04%     
==========================================
  Files         125      125              
  Lines       20547    20538       -9     
==========================================
- Hits        13045    13033      -12     
- Misses       6666     6667       +1     
- Partials      836      838       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zirain
Copy link
Member

zirain commented Mar 1, 2024

I recall there's an improment in upstream go, but I cannot recall which version and commit, can you link it?

Copy link
Member

@cnvergence cnvergence left a comment

Choose a reason for hiding this comment

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

lgtm

@Xunzhuo
Copy link
Member Author

Xunzhuo commented Mar 5, 2024

@zirain I did not get, and it should be none-related to this PR.

Can we get this in ?

@Xunzhuo Xunzhuo requested review from zhaohuabing, qicz and zirain March 5, 2024 06:11
@zhaohuabing
Copy link
Member

zhaohuabing commented Mar 5, 2024

I guess it's related to the variable scope in loop: https://go.dev/blog/loopvar-preview

For Go 1.22, we plan to change for loops to make these variables have per-iteration scope instead of per-loop scope.

It's fixed in Go 1.22, but why lint removes this check in 1.21 ?

@zirain
Copy link
Member

zirain commented Mar 5, 2024

I guess it's related to the variable scope in loop: https://go.dev/blog/loopvar-preview

For Go 1.22, we plan to change for loops to make these variables have per-iteration scope instead of per-loop scope.

It's fixed in Go 1.22, but why lint removes this check in 1.21 ?

yeah, that's what I mean.

should we hold this until we move to go1.22?

@Xunzhuo
Copy link
Member Author

Xunzhuo commented Mar 5, 2024

Sure, let us hold for bumping go version.

@zirain zirain added the hold do not merge label Mar 5, 2024
Copy link

github-actions bot commented Apr 4, 2024

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions!

@github-actions github-actions bot added stale and removed stale labels Apr 4, 2024
@zirain zirain merged commit b608831 into envoyproxy:main Apr 11, 2024
17 checks passed
@shawnh2
Copy link
Contributor

shawnh2 commented Apr 22, 2024

This loopvar feature of go 1.22 requires enable env GOEXPERIMENT=loopvar (default closed) explicitly. Maybe we could consider enable this feature for other places as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hold do not merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants