Skip to content

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