Open Antonboom opened 11 months ago
As mentioned in #2 this linter is more about fmt.Sprint
than strconv
Where do you see strconv.Quote
or strconv.Unquote
being faster than some current code ?
FormatComplex
and FormatFloat
looked more powerful functions with their precision argument than something optimized to be fast.
Do you have examples ?
Do you have examples ?
The first ones that came to mind:
fmt.Sprintf(`"%s"`, "hello")
fmt.Sprintf("%q", "hello")
strconv.Quote("hello")
`"` + "hello" + `"`
fmt.Sprintf("`%s`", "hello")
fmt.Sprintf("%#q", "hello")
"`" + "hello" + "`"
fmt.Sprintf("%q", 91)
"'['"
strings.Trim(`"hello"`, `"`)
strconv.Unquote(`"hello"`)
fmt.Sprintf("%f", 42.42) // And a lot of other verbs.
strconv.FormatFloat(42.42, 'f', 6, 64)
fmt.Println(fmt.Sprintf("%e", 42i+42)) // And other verbs.
fmt.Println(strconv.FormatComplex(42i+42, 'e', 6, 64))
Not all are equivalent cf https://go.dev/play/p/9s4GhmIBEz_J
What I see in some codebase is fmt.Sprintf("constantstring%s" like 0x%x
Offtop:
Please, make a PR in the next time :) It is more suitable for review
Offtopic:
The latest version (v0.2.2) contains features and breaking changes but the version was increased as a bug fix. Your linter is used as a library, so all the exported elements are a part of the public API and should not changed without clear information about them.
It's better for us if you follow semver:
Thanks @ldez I will do a 0.3 with some more features
I did 0.2.1 as suggested here https://github.com/catenacyber/perfsprint/issues/7#issuecomment-1776891993 but @Antonboom did not have the full context I guess
Yes, sorry for the confusion, I suggested a patch version because I didn't consider the flag as a new feature (with a default value equal to the existing behavior). I didn't know about other changes.
@catenacyber about https://github.com/catenacyber/perfsprint/commit/ea8b325cd6f75097103b8c9c9f99902743aea94f
1) I left some comments 2) It looks like thing that is not related to your linter
fmt.Errorf
in any case for consistency. But set of linters checks is not configurable.Also we already have the similar checks:
"%s"
oO)do not define dynamic errors, use wrapped static errors instead
)And this is another one reason for pull requests instead of commits.
Anyway, as u wish.
The change is questionable, because sometimes people use fmt.Errorf in any case for consistency. But set of linters checks is not configurable.
Ok, adding an option to configure this
Usually error path is not performance bottleneck, because it happens much less often.
Indeed, and I hope fmt.Sprintf("%d"
is neither a performance bottleneck. But 0.5% CPU improvement is always good to take
https://staticcheck.dev/docs/checks/#S1028 https://github.com/mgechev/revive/blob/master/RULES_DESCRIPTIONS.md#errorf
Eheh, perfsprint will optimize the fmt.Sprintf
inside errors.New first
Interesting
Looks a different class
And this is another one reason for pull requests instead of commits.
I have been doing that since
Hi!
Maybe you will be interested in: