[WIP] Operations for Quantity references #307
Conversation
|
This is definitely moving in the right direction. My biggest concern so far is duplicate code. The code base already has near-duplicate implementations because of Next is that some of the underlying storage types such as Finally should we be trying to forward to the same operator? e.g. |
|
Apologies for the long delay, my time to work on this is sporadic. I agree, the code duplication is quite horrendous. When I try to make sense of it, I get lost and my eyes glaze over. I've taken the liberty of summarizing your comment, broken down into enumerated questions:
Faced with all of these at once, I don't know where to begin. Do you think we could identify one of them as a candidate on which to start making some progress? |
|
I've been doing some investigation and believe we can eliminate all of the duplication, include the existing duplication for I wrote out some example code in the playground showing how type parameters and references can be inserted and I believe we can put together a recursive macro as follows:
#[cfg(not(feature = "autoconvert"))]
impl_ops!(U);
#[cfg(feature = "autoconvert")]
impl_ops!(Ul, Ur);
macrol_rules! impl_ops {
($(U:ty),+) => {
impl_ops!(@types ($($U),+), ((V, V), (V, &V), (&V, V), (&V, &V));
};
}
macrol_rules! impl_ops {
(@types ($($U:ty),+), ($(($Vl:ident, $Vr:ident)),+)) => {
impl_ops!(@impl ($($U),+), ($(($Vl, $Vr)),+), Add, add, +, ...);
impl_ops!(@impl ($($U),+), ($(($Vl, $Vr)),+), Sub, sub, -, ...);
};
}
macrol_rules! impl_ops {
(@types ($($U:ty),+), ($(($Vl:ident, $Vr:ident)),+), ...) => {
impl<D, $(U,)+, V> $AddMul<Quantity<D, impl_ops!(@Ur $($U),+), impl_ops!(@Vr $($Vl, $Vr),+)> for ...
};
}
macrol_rules! impl_ops {
(@Ul $U:ty) => { $U };
(@Ul $Ul:ty, $Ur:ty) => { $Ul };
(@Ur $U:ty) => { $U };
(@Ur $Ul:ty, $Ur:ty) => { $Ur };
(@Vl $Vl:ident, $Vr:ident) => { $Vl };
(@Vr $Vl:ident, $Vr:ident) => { $Vr };
}Let me know how you want to proceed and how I can assist. You might want to start out with a simple playground or test project and use |
|
So you can think of the whole recursive macro invocation graph as a tree, with macro invocations at the nodes, and actual code ( I think that the way forward is to start at the bottom node-level, and manually invoke the macro to generate a bunch of related leaves. Then repeat it for some siblings of the node, to make sure the pattern is consistent. And then try to go up one level, and replace the manual invocations at the lowest node level, with ones generated by a manual invocation at the higher level. And so on, until the whole lot is replaced with one big top-level invocation. This should make it possible to gradually replace (and fill gaps in) the existing code while keeping the whole lot compiling throughout. Does this sound reasonable? Can you see any obvious flaws in this strategy? Can you think of a good candidate for a first bottom node to try? |
|
That sounds like a good way to start to me. I would just make sure that you include all the parameters that will eventually be needed ( macro_rules! impl_ops {
(($($U:ident),+), $Vl:ty, $Vr:ty, $AddSubTrait:ident, ...) => {
impl<D, $($U,)+ V> $crate::lib::ops::$AddSubTrait<Quantity<D, impl_ops!(@Ur $($U),+), $Vr>> for Quantity<D, impl_ops!(@Ul $($U),+), $Vl> {
// ...
})+
} |
|
Could you help me make sense of the existing implementations? Looking at the original code, I see the following nine
The first four only use add-sub related names, and cover the four possible combinations of assignment-autoconversion. This all makes sense. The next four are trickier:
Then there is a ninth I would have expected autoconversion only to make sense in the presence of |
|
|
I have realistic hope of being able to spend some time on this, this week. I've been wondering about what you wrote here:
I understand how these macro tricks work, but I don't understand why we need them. What does the version on the first line here impl<X, Y> T<x!(@R X, Y)> for S1<x!(@P X, Y)> {}
impl<X, Y> T< Y > for S2< &X > {}(playground) give us, that the version on the second line does not (apart from noise and obfuscation)? Perhaps I'm missing something about how this information will propagate from the top of the macro call, but out of that context it seems that the first line uses Also, I've been wondering whether |
|
Do you have any words of wisdom on how to test this? I've found Lines 54 to 129 in d55d64a Lines 135 to 188 in d55d64a uom/src/si/thermodynamic_temperature.rs Lines 176 to 235 in d55d64a I have used Haskell's Quickcheck, many moons ago and I use Proptest in Rust, but I've never used Rust's Quickcheck, so these tests are a bit cryptic to me. I'm tempted to write some very simple example-based tests, to try to make it blatantly obvious exactly what is being tested, and what isn't in each test. Even if we don't keep them, they should serve as a useful crutch during development. |
|
IIUC
Consequently, I would expect variations on this theme to compile #[test]
fn hmm() {
use crate::si::time::second;
let base_f32 = crate::si::f32::Time::new::<second>(1.0);
let base_f64 = crate::si::f64::Time::new::<second>(1.0);
base_f32 + base_f64;
}but I always get errors along these lines What am I doing wrong? |
|
Responding to your first comment:
The intention was to parameterize if the reference on the type. When I was roughly writing out the example match arms in #307 (comment) I had it in my mind that the reference would be applied to the Here is another playground that I hope better illustrates what I believe needs to be done. To rephrase some of the earlier example code the parameterization should probably look something like this with the different Let me know if this part makes sense. I'll respond to the rest of the questions later today or tomorrow. |
I'm leaning towards a single macro with multiple "private" match arms.
I haven't thought about the best way to organize tests for all of the different combination of ops impls. For reference the first block you linked is for when units with different base units are operated on. The second is the main set of tests for the ops. The third is for temperature specific explicit implementations (certain ops are not implemented for thermodynamic temperature because of it's kind).
|
|
I'm trying to write an exhaustive set of simple-to-understand tests, covering all the things that should work. When I have all of these written out by hand, I'll try to write macros to generate them. These should have much in common, structurally, with the macros For now works, but fails to compile. [And similarly for Is the latter not supposed to work, or am I making a mistake somewhere? |
|
Good catch. Lines 542 to 554 in 10d9679 I don't see why it couldn't explicitly be implemented for impl<D, Ul, Ur, V> $crate::lib::ops::$MulDivAssignTrait<Quantity<DN<Z0>, Ur, V> for Quantity<D, Ul, V>
where ...
{
...
}Lines 274 to 276 in 10d9679 |
jacg
force-pushed
the
quantity-reference-ops
branch
from
891aa9e
to
4de23e8
Compare
|
I've pushed a commit which adds some very concise tests, whose purpose is to summarize what operations currently work. Nothing fancy, no exhaustive property-based testing, just a single hand-picked example for each case, and some macros to maximize the signal-to-noise ratio in the test collection, so that it can act as a quick overview for a human reader. The layout is aggressively aligned to make parsing and comparison by human eye as easy as possible, so it will make any rustfmt checks have a fit, but those are irrelevant for the time being. Could you glance over Lines 95 to 124 in 4de23e8 to check that I have tested everything that you expect to work at present, that the test names correctly reflect what is being tested, and that the numbers in the comments at the end of each line match the relevant implementations listed in #307 (comment). As noted earlier, the assignment operators with Would a small PR for the currently missing |
jacg
force-pushed
the
quantity-reference-ops
branch
3 times, most recently
from
286c1ee
to
f5239ce
Compare
jacg
force-pushed
the
quantity-reference-ops
branch
from
f5239ce
to
a5528dc
Compare
jacg
force-pushed
the
quantity-reference-ops
branch
from
de703fb
to
3805b37
Compare
|
I have added (in the latest commit, at present) impl<D, Ul, Ur, V> $crate::lib::ops::$MulDivAssignTrait<Quantity<DimensionOne, Ur, V>> for Quantity<D, Ul, V>
where ...but the relevant tests continue to fail to compile with exactly the same error message as before: Any idea why it's not picking up this new implementation as appropriate for the requested operation? |
|
A separate PR to add Took me a while of staring at the code, but I believe the following two changes should get things compiling. 8c8
< + $crate::lib::ops::$MulDivAssignTrait<Quantity<DimensionOne, Ur, V>>,
---
> + $crate::lib::ops::$MulDivAssignTrait<V>,
12c12,13
< self.value $muldivassign_op rhs;
---
> self.value $muldivassign_op rhs.value;
> // change_base is needed for autoconvert! version. |
Done in #364. |
Fix #38.
Prototype implementations of
&q +/- qq +/- &q&q +/- &qCould you check that I'm barking up the right tree, before I try to complete the rest?