Skip to content
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

Why is replace_consts enabled by default? #2380

Closed
gnzlbg opened this issue Jan 18, 2018 · 7 comments
Closed

Why is replace_consts enabled by default? #2380

gnzlbg opened this issue Jan 18, 2018 · 7 comments
Labels
A-documentation Area: Adding or improving documentation good-first-issue These issues are a good way to get started with Clippy

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 18, 2018

The replace_consts lint is enabled in clippy-pedantic by default.

The docs hints that using consts is bad because const fn exists.

This does not make any sense to me: why should const fn be preferred over consts? Are associated consts also bad?

@oli-obk
Copy link
Contributor

oli-obk commented Jan 18, 2018

Are associated consts also bad?

no, but there are none ;)

std::i32::MAX is not i32::MAX, but i32::max_value() is on the type

This does not make any sense to me: why should const fn be preferred over consts?

the const fns for integers are disputed, but the const fns for atomics are the way to go, the old constants were hacks that existed because of the lack of const fn.

see also the discussion on the PR adding this lint #2344

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jan 18, 2018

FWIW I got this error on std::usize::MAX.

but the const fns for atomics are the way to go

This makes sense.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jan 18, 2018

Maybe what this lint needs is just better docs? The argument that ::std::usize::MAX needs the whole path, but usize::max_value() does not because it works on the type is a good one actually.

@oli-obk oli-obk added good-first-issue These issues are a good way to get started with Clippy A-documentation Area: Adding or improving documentation labels Jan 18, 2018
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jan 19, 2018

So I think the main arguments are:

  • it is more idiomatic to directly associate constants with a type than to put them in a module that only contains the type using associcated consts or const fn
  • this allows you to use the constant without the full path, like so usize::max_value().
  • now that associated consts are stable there is discussion about moving these constants into these types, and deprecating the module level ones (so using the module level ones should not be encouraged)

@petrochenkov you had strong thoughts about this. How would you document this?

nickdrozd added a commit to nickdrozd/remacs that referenced this issue Jun 12, 2019
There are a few changes packed into this one.

- Cut unnecessary negations (mostly shuffling if/else blocks)
- Use isize::min_value() instead of std::isize::MIN, etc
    - rust-lang/rust-clippy#2380
- Rename some shadowing variables
- Cut some unnecessary references
- Use <struct>::default instead of Default::default
- Update inclusive range syntax
- Cut open import
- Self
- dyn trait
nickdrozd added a commit to nickdrozd/remacs that referenced this issue Jun 12, 2019
There are a few changes packed into this one.

- Cut unnecessary negations (mostly shuffling if/else blocks)
- Use isize::min_value() instead of std::isize::MIN, etc
    - rust-lang/rust-clippy#2380
- Rename some shadowing variables
- Cut some unnecessary references
- Use <struct>::default instead of Default::default
- Update inclusive range syntax
- Cut open import
- Self
- dyn trait
nickdrozd added a commit to nickdrozd/remacs that referenced this issue Jul 18, 2019
There are a few changes packed into this one.

- Cut unnecessary negations (mostly shuffling if/else blocks)
- Use isize::min_value() instead of std::isize::MIN, etc
    - rust-lang/rust-clippy#2380
- Rename some shadowing variables
- Cut some unnecessary references
- Use <struct>::default instead of Default::default
- Update inclusive range syntax
- Cut open import
- Self
- dyn trait
@phansch
Copy link
Member

phansch commented Mar 5, 2020

rust-lang/rust#68952 stabilized the assoc_int_consts feature for Rust 1.34. For Clippy this is relevant for at least the replace_consts lint:

As far as I can tell, that lint was meant to suggest to use the associated methods on the integer primitives, instead of the constants that have now been soft-deprecated in rust-lang/rust#68952. Although, the lint doesn't really provide a reason why one is better over the other. (Please correct me if I misunderstood that lint)

The RFC also mentions a deprecation of the associated functions the lint is currently suggesting, so we might want to deprecate replace_consts altogether.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 5, 2020

We can keep it but suggest moving from module consts to associated constant instead of to associated functions

@tesuji
Copy link
Contributor

tesuji commented Apr 1, 2020

This could be closed now with #5380.

@oli-obk oli-obk closed this as completed Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documentation Area: Adding or improving documentation good-first-issue These issues are a good way to get started with Clippy
Projects
None yet
Development

No branches or pull requests

5 participants