Open AaronKutch opened 6 years ago
For brevity in internal tests, should I add a bw!() macro which creates and unwraps BitWidths? The different syntax highlighting and bang should make it visible enough to be an exception. Should we make it public?
No please keep it private at first.
I have had a lot of thinking over the past month about
apint
, and I think I will put most of it here. These are things I had off the top of my head that I had to type down, maybe I will remember more.Calling
BitWidth::new(...).unwrap()
just to handle the 0 width case outside the function might seem to have barely any performance increase when dynamically setting a variable with it and then calling several constructors with that one variable, but I think doing many small things like it will make a significant difference. In benchmarks I just did, we are outperformingramp
by about 0.4x on basic ops, and I think it is because of many small branching choices I am making in the newarithmetic.rs
and because of all your small design choices working together to make a big difference. For brevity though, I think we should make a macro, perhapsbw!()
, and make it a procedural macro so that it produces compilation errors on 0. We could also use the procedural macro crate for future compile time stuff.BitWidth::new()
should be kept for dynamic bit size purposes, and I wonder if we should get rid of the other BitWidth constructors since they have the unwrap inside them, and I much prefer to be able to seeunwrap
s and mess withResult
s. We obviously cannot do the same thing withShiftAmount
but we should keep that,BitPos
, andRadix
for purposes of easily distinguishing between widths and positions and shifts and them having special purpose methods. Thorough documentation could also be included for each of these types later.I had some trouble getting around the crate for a long while, and I think making the following organizational changes would help alot. Make sure that all tests pass before doing this, change to
edition = 2018
,cargo fix
ituint.rs
andint.rs
where they are. They are just complicated wrappers aroundApInt
and almost no impls for them should be anywhere else.mod.rs
should be changed toapint.rs
. The extremely unsafe and important stuff like the Drop impls inconstructor.rs
and the Clone impl incasting.rs
should be moved here.apint/utils.rs
should be moved toserialization.rs
is_one
,is_even
, etc inapint/utils.rs
should be put inrelational.rs
utils.rs
should be merged withapint/utils.rs
ShiftAmount
and what is inchecks.rs
and put them in a new fileshiftamount.rs
.shiftamount.rs
,bitpos.rs
,bitwidth.rs
,radix.rs
,traits.rs
, anderror.rs
in their own folder, maybe calledinfo
.apint.rs
,int.rs
,uint.rs
,digit.rs
,digit_seq.rs
are put into a folder calleddata
arithmetic.rs
,casting.rs
,constructors.rs
,relational.rs
,shift.rs
,bitwise.rs
, andutils.rs
are put into a folder calledlogic
rand_impl.rs
,serialization.rs
,serde_impl.rs
,to_primitive.rs
,lib.rs
, and the folders are what remain at the top level ofsrc
A while ago I raised an issue about the Clone impl you wrote for
apint
but benchmarks show that it is performing as quickly as whatramp
has, so unless we happened to end up with a same less than optimal cloning routine, it is good.I just realized something. The impl for ApInt looks like:
How does this work? wouldn't the
ext
have to beNonNull<Storage>
?