Consolidate range macros

[mulkieran]

• Isolate them into one spot for easier refactoring later
• Distinguish between usize and other unsigned macros. usize can be larger than u64, so rust does not allow
  the guaranteed succesful u64::from when converting a usize to a u64.
• Get rid of long comment and one clippy allow in favor of zero comments and several clippy allows on exactly the things that need to be allowed.
• Add some tests to verify that the interface provided is as expected.

[mulkieran]

This step precedes the step where I make a submodule and put the core modules in a submodule called "core". That step is in progress: #421.

[mulkieran]

Have verified that current master of stratisd compiles against these changes.

[tasleson]

Macro's are not an easy beast to review IMHO, so I stubbed up some code:

use devicemapper::Bytes;

fn main() {

    // This doesn't work?
    /*
    let array = vec!(Bytes(0), Bytes(1), Bytes(2));
    let sum: Bytes = array.iter().sum();
    assert_eq!(sum, Bytes(3));
    */

    assert_eq!(Bytes(100) / Bytes(10), 10u64);              // This works, but inconsistent with others
    // assert_eq!( Bytes(100)/Bytes(10), Bytes(10));        // Not supported?

    assert_eq!(Bytes(100) / 10u64, Bytes(10));
    assert_eq!(Bytes(100) / 10usize, Bytes(10));

    assert_eq!(Bytes(100) * 10u64, Bytes(1000));
    assert_eq!(Bytes(100) * 10usize, Bytes(1000));

    assert_eq!(10u64 * Bytes(100), Bytes(1000));
    assert_eq!(10usize * Bytes(100), Bytes(1000));


    //assert_eq!( Bytes(10) * Bytes(10), 100u64);           // Not supported, but is for / ?
    //assert_eq!( Bytes(10) * Bytes(10), Bytes(100));       // Not supported ?

    assert_eq!(10usize * Bytes(100), Bytes(1000));
    assert_eq!(10u64 * Bytes(100), Bytes(1000));

    assert_eq!(Bytes(100) % 10u64, Bytes(0));
    assert_eq!(Bytes(100) % 10usize, Bytes(0));
    assert_eq!(Bytes(100) % Bytes(10), Bytes(0));

    //assert_eq!(10u64 % Bytes(100), Bytes(0));             // Not supported, but is for others
    //assert_eq!(10usize % Bytes(100), Bytes(0));           // Not supported, but is for others

    assert_eq!(Bytes(100).checked_add(Bytes(10)).unwrap(), Bytes(110));
    assert_eq!(Bytes(18446744073709551615).checked_add(Bytes(1)), None);
}

I'm finding some inconsistencies in what is supported for each of the operators, see comments in code.

I believe we should have a unit test for this macro, to ensure to meets our expectations for syntax and for ensuring that the results are calculated correctly.

[tasleson]

@mulkieran Please elaborate on what an omnibus macro is too. Nevermind, I thought it was some type of special macro, but you're just stating a collection of macros.

[mulkieran]

Macro's are not an easy beast to review IMHO, so I stubbed up some code:

use devicemapper::Bytes;

fn main() {

    // This doesn't work?
    /*
    let array = vec!(Bytes(0), Bytes(1), Bytes(2));
    let sum: Bytes = array.iter().sum();
    assert_eq!(sum, Bytes(3));
    */

    assert_eq!(Bytes(100) / Bytes(10), 10u64);              // This works, but inconsistent with others
    // assert_eq!( Bytes(100)/Bytes(10), Bytes(10));        // Not supported?

    assert_eq!(Bytes(100) / 10u64, Bytes(10));
    assert_eq!(Bytes(100) / 10usize, Bytes(10));

    assert_eq!(Bytes(100) * 10u64, Bytes(1000));
    assert_eq!(Bytes(100) * 10usize, Bytes(1000));

    assert_eq!(10u64 * Bytes(100), Bytes(1000));
    assert_eq!(10usize * Bytes(100), Bytes(1000));


    //assert_eq!( Bytes(10) * Bytes(10), 100u64);           // Not supported, but is for / ?
    //assert_eq!( Bytes(10) * Bytes(10), Bytes(100));       // Not supported ?

    assert_eq!(10usize * Bytes(100), Bytes(1000));
    assert_eq!(10u64 * Bytes(100), Bytes(1000));

    assert_eq!(Bytes(100) % 10u64, Bytes(0));
    assert_eq!(Bytes(100) % 10usize, Bytes(0));
    assert_eq!(Bytes(100) % Bytes(10), Bytes(0));

    //assert_eq!(10u64 % Bytes(100), Bytes(0));             // Not supported, but is for others
    //assert_eq!(10usize % Bytes(100), Bytes(0));           // Not supported, but is for others

    assert_eq!(Bytes(100).checked_add(Bytes(10)).unwrap(), Bytes(110));
    assert_eq!(Bytes(18446744073709551615).checked_add(Bytes(1)), None);
}

I'm finding some inconsistencies in what is supported for each of the operators, see comments in code.

I believe we should have a unit test for this macro, to ensure to meets our expectations for syntax and for ensuring that the results are calculated correctly.

I'm all in favor of having tests, but that ought to be outside the scope of this issue, which adds no new operations. There are a limited number of tests in src/types.rs on some constructed types. These still pass, obviously. Would you like to open an issue for these additional proposed tests or should I? EDIT: Courtesy of the Rust compiler, these macros will always be syntactically correct, so we should not test for syntactic correctness. It is also fairly meaningless to test for correctness of operations; we would just be testing for correctness of u64 operations, which is something we are obliged to rely on if we use Rust. What this PR does enable, though, is a test of the interface that the implementation generated by the range macro must support. I'll give that a try.

[mulkieran]

Macro's are not an easy beast to review IMHO, so I stubbed up some code:

use devicemapper::Bytes;

fn main() {

    // This doesn't work?
    /*
    let array = vec!(Bytes(0), Bytes(1), Bytes(2));
    let sum: Bytes = array.iter().sum();
    assert_eq!(sum, Bytes(3));
    */

It doesn't compile. Here's the error:

type annotations required: cannot resolve `_: std::iter::Sum<&types::Bytes>`

Try adding turbofish ::<Bytes> to help out the compiler. New error is:

the trait bound `types::Bytes: std::iter::Sum<&types::Bytes>` is not satisfied
   --> src/types.rs:253:62
    |
253 |         assert_eq!(vec!(Bytes(0), Bytes(1), Bytes(2)).iter().sum::<Bytes>(), Bytes(3));
    |                                                              ^^^ the trait `std::iter::Sum<&types::Bytes>` is not implemented for `types::Bytes`
    |
    = help: the following implementations were found:
              <types::Bytes as std::iter::Sum>

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
error: Could not compile `devicemapper`.
warning: build failed, waiting for other jobs to finish...
error: build failed
make: *** [Makefile:19: test] Error 101

That's because sum is not defined for &Bytes just for Bytes. Modify the expression:

assert_eq!(vec!(Bytes(0), Bytes(1), Bytes(2)).iter().cloned().sum::<Bytes>(), Bytes(3));

and it compiles. This is the way it was, and perhaps the way it always shall be, see for example the allocated function in src/engine/strat_engine/backstore/data_tier.rs. Feel free to open an issue for this as well, it might be there is something to make this more convenient. But, we've learned to live with it in stratisd, so far.

Regarding the other questions below, they appear to be all covered by the reasoning explained in the justbytes docs: /~https://github.com/mulkieran/justbytes which I'll quote here:

"""
Operations

This module does not accomodate multi-dimensionality of address ranges. Consequently, multiplying one Range object by another Range object will cause an error to be raised, since bytes^2 is not representable by the module. For most uses any operation which would yield a multi-dimensional quantity is not useful. There are no plans to adapt this package so that it can accomodate multi-dimensionality of address ranges.

Numerous computations with address ranges are nonsensical. For example, 2 raised to a power which is some address range, is a meaningless computation. All such operations cause an error to be raised.

Some computations with precise, finite, values may yield irrational results. For example, while 2 is rational, its square root is an irrational number. There is no allowed operation on Range objects which can result in an irrational Range value. It turns out that all such operations are either nonsensical or would result in a value with an unrepresentable type.

The result type of operations is a Range, where appropriate, or a subtype of Rational, where a numeric value is appropriate.
"""

assert_eq!(Bytes(100) / Bytes(10), 10u64);              // This works, but inconsistent with others
// assert_eq!( Bytes(100)/Bytes(10), Bytes(10));        // Not supported?

The appropriate units for Bytes(100) / Bytes(10) is none, so the result to expect is 10, specifically 10u64.

assert_eq!(Bytes(100) / 10u64, Bytes(10));
assert_eq!(Bytes(100) / 10usize, Bytes(10));

assert_eq!(Bytes(100) * 10u64, Bytes(1000));
assert_eq!(Bytes(100) * 10usize, Bytes(1000));

assert_eq!(10u64 * Bytes(100), Bytes(1000));
assert_eq!(10usize * Bytes(100), Bytes(1000));


//assert_eq!( Bytes(10) * Bytes(10), 100u64);           // Not supported, but is for / ?
//assert_eq!( Bytes(10) * Bytes(10), Bytes(100));       // Not supported ?

Bytes(10) * Bytes(10) is 100 bytes^2. This library does not support multidimensionality. Practically speaking, few people try to allocated things in bytes^2, probably because nobody manufactures that kind of storage. This library can't express the result, and few people want it.

assert_eq!(10usize * Bytes(100), Bytes(1000));
assert_eq!(10u64 * Bytes(100), Bytes(1000));

assert_eq!(Bytes(100) % 10u64, Bytes(0));
assert_eq!(Bytes(100) % 10usize, Bytes(0));
assert_eq!(Bytes(100) % Bytes(10), Bytes(0));

//assert_eq!(10u64 % Bytes(100), Bytes(0));             // Not supported, but is for others
//assert_eq!(10usize % Bytes(100), Bytes(0));           // Not supported, but is for others

Modulus has a mathematical definition, closely related to that for division. What we have is pq + r = x where p is the multiplier, q is the quotient, r is the remainder (modulus), and x is the value. The result of x % p is the value r such that there is some q s.t. pq + r =x and r < p. The units have to agree. So, the units of x and the units of r must be the same. In both your examples, x is a plain number, 10u64 or 10usize. Thus, r must also be a plain number, by necessity, not bytes. In both your examples, the units of p is Bytes. That requires, for the units to correspond that the units of the unknown q that must exist be bytes^-1. Multi-dimensionality is not supported by this library, so this operation is not allowed, as the corresponding operation for division can not yield a representable result.

assert_eq!(Bytes(100).checked_add(Bytes(10)).unwrap(), Bytes(110));
assert_eq!(Bytes(18446744073709551615).checked_add(Bytes(1)), None);

}

I'm finding some inconsistencies in what is supported for each of the operators, see comments in code.

All the existing operations are consistent with standard arithmetic and well-established rules about dimensionality and units.

The omnibus macro does not contain a full set of arithmetic operations. Operations have been added incrementally and even somewhat tentatively. If you think there are some necessary additional operations that should be added, please file an issue. We would require them to conform to the established rust standard library traits to be really useful, generally speaking.

[mulkieran]

New issues spawned:
#423
#424
#425

[mulkieran]

Other related issues:
#399
#101
#79

I've filed a whole new bunch of issues, and added some tests

[mulkieran]

@tasleson review, please?

[mulkieran]

Err, I believe it is possible to test Debug and Display as well, so moving back to in-progress.

[mulkieran]

Ok, Debug impl tested.

[mulkieran]

A completely unrelated failure in Jenkins.

[mulkieran]

A removal of a thindev failed in test cleanup because the thindev was still busy. The removal was in the test itself. Possibly the test infrastructure should be the one to handle the removal. Anyway, trying again.

[mulkieran]

One more issue: #426.

[mulkieran]

Ok. I believe it's ready again.

[mulkieran]

@tasleson Please let me know if you have further questions or requests. If not, I'll close this and make up a final version. Actually, the commits all look good as is. So I'ld treat this as the final version.

[tasleson]

@mulkieran IMHO this should be squashed.

[mulkieran]

@mulkieran IMHO this should be squashed.

I can reduce it to a few fewer commits. But I don't think it should be squashed altogether.

[mulkieran]

Superceded by #427.