Skip to content

dev: refactor ifs with cmp.Or #5194

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

Merged
merged 6 commits into from
Dec 6, 2024
Merged

Conversation

alexandear
Copy link
Member

The PR refactors applicable if statements to use cmp.Or.

See https://blog.carlana.net/post/2024/golang-cmp-or-uses-and-history/

@alexandear alexandear added the topic: cleanup Related to code, process, or doc cleanup label Dec 6, 2024
@ldez ldez self-requested a review December 6, 2024 12:07
@ldez
Copy link
Member

ldez commented Dec 6, 2024

This is an abuse of cmp.Or: this function is mainly useful when creating a comparison implementation.
The readability and performance of cmp.Or are worse than simple if.

First, because this function iterates through a slice.

Secondly, cmp.Or requires the evaluation of all parts before comparing them whereas the if evaluates the first and the second only if needed, so this is mainly a bad idea to put this everywhere.

For example:

l.cfg.Run.Go = cmp.Or(l.cfg.Run.Go, detectGoVersion())

In this example, detectGoVersion() will be always called, but we don't want that because this call is costly.

With an if the function detectGoVersion() is only called if needed.

if l.cfg.Run.Go == "" {
    l.cfg.Run.Go = detectGoVersion()
}

So cmp.Or should be used only for values, and when this really improves the readability.

@ldez ldez added topic: cosmetic changes contain cosmetic improvements and removed topic: cleanup Related to code, process, or doc cleanup labels Dec 6, 2024
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

I hope the usage of cmp.Or will not be wrongly widespread over the Go ecosystem.

@ldez ldez merged commit 7b15252 into golangci:master Dec 6, 2024
15 checks passed
@alexandear alexandear deleted the refactor/cmp-or branch December 6, 2024 13:11
@ccoVeille
Copy link
Contributor

I hope the usage of cmp.Or will not be wrongly widespread over the Go ecosystem.

Maybe it's time to build a new linter 🦸

@ccoVeille

This comment was marked as off-topic.

@ldez ldez modified the milestones: next, v1.63 Dec 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: cosmetic changes contain cosmetic improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants