Add lint to replace consts with const fns
#2344
Conversation
| (&["core", "sync", "atomic", "ATOMIC_U32_INIT"], "AtomicU32::new(0)"), | ||
| (&["core", "sync", "atomic", "ATOMIC_U64_INIT"], "AtomicU64::new(0)"), | ||
| // Min | ||
| (&["core", "isize", "MIN"], "isize::min_value()"), |
There was a problem hiding this comment.
MIN/MAX are not bad at all, I'd rather recommend the opposite replacement.
There was a problem hiding this comment.
I'll add a config option for enabling MIN/MAX on this lint. The opposite replacement might be better suited for a different lint though.
There was a problem hiding this comment.
@petrochenkov why? the MIN is in std::isize::MIN and thus requires the full path or an import. min_value exists on the type
There was a problem hiding this comment.
That's actually the main reason I recommend these; fewer paths to import.
There was a problem hiding this comment.
I sort of assumed that it'd make sense to long-term push people toward the const fn versions over the constants. For the INIT structs, they simply aren't at clear as using new, and for the MIN/MAX structs, the types themselves are already in scope.
There was a problem hiding this comment.
the
MINis instd::isize::MINand thus requires the full path or an import.
Oh. I thought they are associated constants already.
Someone should resurrect that PR.
I sort of assumed that it'd make sense to long-term push people toward the const fn versions over the constants.
Why? Constants exist in the language to represent... constants. MIN/MAX are constants, why would you avoid a feature specifically designed to represent them (except for the full path issue which is a fixable library deficiency). min_value() is longer and has that extraneous call.
(I agree about INIT atomics though.)
There was a problem hiding this comment.
^^ This was a concern about constants and const functions in general.
All the listed constants have specific reasons to avoid them (until rust-lang/rust#28656 is redone), so this PR is okay.
There was a problem hiding this comment.
I totally agree with you @petrochenkov in terms of making them associated constants. When you do end up redoing that PR, you should include the float constants too, as those don't have associated const fns.
There was a problem hiding this comment.
So, no config option for MIN/MAX then?
There was a problem hiding this comment.
Nah, leave it out, we'll adjust the lint when it makes sense to behave differently on stable
Fixes #2288
I think the "Why is this bad" section in the documentation should have more, but I'm not sure what to put there.