Why is replace_consts enabled by default? #2380

Activity

1. oli-obk commented on Jan 18, 2018

   

   oli-obk

   on Jan 18, 2018

   Contributor

   More actions

   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
2. gnzlbg commented on Jan 18, 2018

   

   gnzlbg

   on Jan 18, 2018

   ContributorAuthor

   More actions

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

   but the const fns for atomics are the way to go

   This makes sense.
3. gnzlbg commented on Jan 18, 2018

   

   gnzlbg

   on Jan 18, 2018

   ContributorAuthor

   More actions

   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.
4. oli-obk

   added

   good first issueThese issues are a good way to get started with ClippyThese issues are a good way to get started with Clippy

   A-documentationArea: Adding or improving documentationArea: Adding or improving documentation

   on Jan 18, 2018
5. gnzlbg commented on Jan 19, 2018

   

   gnzlbg

   on Jan 19, 2018

   ContributorAuthor

   More actions

   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?
6. Manishearth

   added

   hacktoberfest

   on Oct 9, 2018
7. nickdrozd

   added 2 commits that reference this issue on Jun 12, 2019

   Yet more Clippy

   ...

   372754a

   Yet more Clippy

   ...

   f0afcfc
8. nickdrozd

   added a commit that references this issue on Jul 18, 2019

   Yet more Clippy

   ...

   6eba07a
9. phansch commented on Mar 5, 2020

   

   phansch

   on Mar 5, 2020

   Last edited by phansch

   Contributor

   More actions

   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.
10. oli-obk commented on Mar 5, 2020

    

    oli-obk

    on Mar 5, 2020

    Contributor

    More actions

    We can keep it but suggest moving from module consts to associated constant instead of to associated functions
11. tesuji commented on Apr 1, 2020

    

    tesuji

    on Apr 1, 2020

    Contributor

    More actions

    This could be closed now with #5380.
12. oli-obk

    closed this as completedon Apr 1, 2020