Skip to content

fix: ineffective assignments#650

Open
alrs wants to merge 1 commit into
pkg:masterfrom
alrs:ineffective-functions
Open

fix: ineffective assignments#650
alrs wants to merge 1 commit into
pkg:masterfrom
alrs:ineffective-functions

Conversation

@alrs
Copy link
Copy Markdown

@alrs alrs commented May 15, 2026

This fixes two functions that had a non-pointer receiver, preventing them from effectively updating fields on the orderedResponse{} and orderedRequest{} structs.

In looking at this, I realize that both of these functions are unused. I'd be happy to take the next step and remove them both.

Copy link
Copy Markdown
Collaborator

@puellanivis puellanivis left a comment

Choose a reason for hiding this comment

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

Great catch! It’s probably because they’re unused that the bug was never spotted.

Just two tiny one-character pieces of lint, and 🤷‍♀️ yeah, if they’re unused, we can at least try deleting them. Being that the type itself is unexported, there’s really no reason to implement setters like this unless it needs to match an interface, and the interface doesn’t contain it… so… yeah. I say, chuck it and then we see what breaks. (Easy enough to put back.)

Comment thread packet-manager.go Outdated
}
func (p orderedRequest) orderID() uint32 { return p.orderid }
func (p orderedRequest) setOrderID(oid uint32) { p.orderid = oid }
func (p orderedRequest) orderID() uint32 { return p.orderid }
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If we’re changing one receiver to a pointer receiver, then we should generally convert all of them to pointer receivers. (Unless there’s a good reason, like how lots of shallow clones are simply value receivers that return a pointer to their receiver… but now with new(*myPointer) not sure how useful that pattern is anymore.)

@alrs alrs force-pushed the ineffective-functions branch from c89869b to dbf7c75 Compare May 19, 2026 00:23
@alrs
Copy link
Copy Markdown
Author

alrs commented May 19, 2026

I've rebased the commit to remove the two orderID() functions.

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