Factoring bigint, rational, and complex out of libextra into libnum. #12154
Conversation
|
This is the revised version of PR #12141 ; note that there was some discussion on that PR regarding the motivation for this change. (The big one being enabling faster iteration on bigint improvements such as pnkfelix/rust@d669822 ) In particular, I continue to assert that the decision of whether to change the defaults so that The question of whether to remove |
|
|
||
| pub mod bigint; | ||
| pub mod rational; | ||
| pub mod complex; |
There was a problem hiding this comment.
We've been moving towards private inner modules and reexports of their contents, so perhaps these could be mod foo; with pub use bigint::{BigInt, BigUint}; etc at the top?
There was a problem hiding this comment.
Hmm. When importing via globs, it is nicer I think to have these things actually collected together like this.
But I can see that this does lead to the stuttering problem.
Is it reasonable to re-export the contents from the num crate, but to have the inner modules be public, to allow such usage? Or is that just asking for trouble?
There was a problem hiding this comment.
That's a good point, I suppose globs really do want to import num::bigint::*, not num::*. Then again, globs are feature-gated...
One part about pub mod foo; is that the reexport doesn't have its documentation inlined at the reexport site, but rather it's a redirect. That's not necessarily a downside, but it is something to consider.
Let's go with this for now, and see how it turns out.
|
r=me with a squash |
Removed use of globs present in earlier versions of modules. Fix tutorial.md to reflect `extra::rational` ==> `num::rational`.
…-libextra, r=alexcrichton Removed use of globs present in earlier versions of modules. Fix tutorial.md to reflect `extra::rational` ==> `num::rational`.
|
Odd, I was able to compile pidigits after applying my original version of this PR (#12141), but now that this has landed in master, my direct invocations of And worse, explicitly trying to help (edit: Ah: I was trying to run (edit^2: wait, that command line above isn't running |
|
That should be fixed by #12164 (the assertion error) |
[`unconditional_recursion`]: compare by `Ty`s instead of `DefId`s Fixes rust-lang#12154 Fixes rust-lang#12181 (this was later edited in, so the rest of the description refers to the first linked issue) Before this change, the lint would work with `DefId`s and use those to compare types. This PR changes it to compare types directly. It fixes the linked issue, but also other false positives I found in a lintcheck run. For example, one of the issues is that some types don't have `DefId`s (primitives, references, etc., leading to possible FNs), and the helper function used to extract a `DefId` didn't handle type parameters. Another issue was that the lint would use `.peel_refs()` in a few places where that could lead to false positives (one such FP was in the `http` crate). See the doc comment on one of the added functions and also the test case for what I mean. The code in the linked issue was linted because the receiver type is `T` (a `ty::Param`), which was not handled in `get_ty_def_id` and returned `None`, so this wouldn't actually *get* to comparing `self_arg != ty_id` here, and skip the early-return: https://github.com/rust-lang/rust-clippy/blob/70573af31eb9b8431c2e7923325c82ba0304cbb2/clippy_lints/src/unconditional_recursion.rs#L171-L178 This alone could be fixed by doing something like `&& get_ty_def_id(ty).map_or(true, |ty_id)| self_arg != ty_id)`, but we don't really need to work with `DefId`s in the first place, I don't think. changelog: [`unconditional_recursion`]: avoid linting when the other comparison type is a type parameter
Removed use of globs present in earlier versions of modules.
Fix tutorial.md to reflect
extra::rational==>num::rational.