Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

chore: remove unnecessary redeclarations in for loops#18440

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
aslilac merged 5 commits intomainfromlilac/go-122-range-fix
Jun 20, 2025

Conversation

aslilac
Copy link
Member

disclaimer: I asked claude 4 to do this little bit of cleanup.

I noticed we still have a bunch of these, but these kinds of re-declarations are no longer necessary now that we live in a post go 1.22 world. not a huge change, but didn't take claude very long. :p

Copy link
Member

@johnstcnjohnstcn left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! I checked and as far as I can tell this should all be safe. If we wanted to be super-safe, we'd just do the ones in*_test.go and leave the rest till after code freeze.
There are still a few straggling comments left that can also probably be removed.

@aslilacaslilacforce-pushed thelilac/go-122-range-fix branch from40fd4ac tobf077d3CompareJune 18, 2025 22:05
@aslilac
Copy link
MemberAuthor

sorry about that, didn't expect anyone to look so quickly. just cleaned the diff up a bunch.

@aslilacaslilac changed the titlechore: remove unnecessary redelarations in for loopschore: remove unnecessary redeclarations in for loopsJun 18, 2025
johnstcn
johnstcn previously approved these changesJun 19, 2025
Copy link
Member

@johnstcnjohnstcn left a comment•
edited
Loading

Choose a reason for hiding this comment

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

All of these loopvar instances look safe to remove.

EDIT: for an abundance of safety, I'd prefer if weonly did this for*_test.go and left the rest alone.

dannykopping reacted with thumbs up emoji
Comment on lines 411 to 413
// When we assign the non-pointer to a
// variable it loses the reference.
replica:=replica
Copy link
Member

Choose a reason for hiding this comment

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

This is the only instance I'm slightly wary of, but the new behaviour seems to be as intended here. It was originally added in#4596 to avoid a test race.

@johnstcnjohnstcn dismissed theirstale reviewJune 19, 2025 08:06

*_test.go please

@aslilac
Copy link
MemberAuthor

by my count, only 14 of the 214 files this touchesaren't tests.

  • agent/agent.go
  • agent/agentscripts/agentscripts.go
  • cli/organizationroles.go
  • cli/organizationsettings.go
  • cli/server.go
  • coderd/database/pubsub/pubsub_memory.go (really, even this could probably be counted as a test file)
  • coderd/externalauth/externalauth.go
  • coderd/idpsync/group.go
  • coderd/rbac/authz.go (this one can easily be seen to have never needed the redeclaration to begin with)
  • coderd/rbac/roles.go
  • coderd/webpush/webpush.go
  • enterprise/coderd/proxyhealth/proxyhealth.go
  • enterprise/replicasync/replicasync.go (the one you commented on)
  • site/site.go

this language change was very well documented. all this does is remove redundant copies, sincefor loops are now well defined to give you an appropriately scoped copy to begin with. I'm all for caution but I don't see what it would gain us here.

@aslilacaslilacforce-pushed thelilac/go-122-range-fix branch frome98caae toef8ddeeCompareJune 20, 2025 18:46
@aslilacaslilac requested a review fromjohnstcnJune 20, 2025 18:49
@aslilacaslilac merged commitfae30a0 intomainJun 20, 2025
37 checks passed
@aslilacaslilac deleted the lilac/go-122-range-fix branchJune 20, 2025 19:16
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJun 20, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@johnstcnjohnstcnjohnstcn approved these changes

@spikecurtisspikecurtisAwaiting requested review from spikecurtisspikecurtis is a code owner

Assignees

@aslilacaslilac

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@aslilac@johnstcn
[8]ページ先頭
©2009-2025 Movatter.jp