sway icon indicating copy to clipboard operation
sway copied to clipboard

Signed integers

Open rostyslavtyshko opened this issue 3 years ago • 8 comments

i8, i16, i32, i64

Basic arithmetic operations and tests

Closes #2422

rostyslavtyshko avatar Jul 17 '22 16:07 rostyslavtyshko

Does this have a related issue?

otrho avatar Jul 18 '22 00:07 otrho

BTW, is there an issue for this? I didn't find one -- could be good to make one for reference so we can document motivating factors.

sezna avatar Jul 30 '22 00:07 sezna

BTW, is there an issue for this? I didn't find one -- could be good to make one for reference so we can document motivating factors.

Added one

rostyslavtyshko avatar Jul 30 '22 07:07 rostyslavtyshko

BTW, is there an issue for this? I didn't find one -- could be good to make one for reference so we can document motivating factors.

Added one

Do you mind further explaining how this library works and how to use it with additional comments (preferably docstrings) and in-depth description in this PR's description or in the corresponding issue? In particular, I'm interested in providing proper documentation for end users as well as some design notes for future maintainers.

mohammadfawaz avatar Jul 31 '22 18:07 mohammadfawaz

  1. I recommend chaning the syntax in the From trait for each type in addition to any impl that uses the name of the type rather than Self e.g.
impl From for I128 {
    /// Helper function to get a signed number from with an underlying
    fn from(value: U128) -> I128 {
        I128 {
            underlying: value,
        }
    }
}

would become

impl From for I128 {
    /// Helper function to get a signed number from with an underlying
    fn from(value: U128) -> Self {
        Self {
            value  // This requires the struct field to be updated. Alternatively, change the param to be "underlying"
        }
    }
}
2. I find the control flow in the `Multiply` & `Divide` implementations to be a little difficult to read. I presume the compiler can optimize away the number of `~Type::indent()` calls but I'm wondering if assigning to a variable, similar to `res`, and then using that variable in the flow is easier to read (perhaps also without an overhead).

3. In the `Sway` tests there are imports using `*`. Are these still needed to import the `impl` blocks?

4. The indentation for the struct declarations is messed up in the following tests
   
   * `i128`
   * `i256`

5. I would personally restructure the tests. I see two approaches.
   
   * Currently, each "test" (assertion) has its data defined directly above it and they flow from 1 test to another. You can definitely see where one test starts and one test ends if you look for it but it's not as quick and simple (imo) as it would be in a unit test function format.
   * The alternative is to define everything in one place (usually at the top) and then keep the tests (assertions) bundled together at the bottom. This does not allow you to shadow variable names so it might be tedious to name things but I think this enforces a higher quality structure and it's what I would prefer to see here. Otherwise, unit tests should be used but that cannot be done in a script atm
  1. Fixed
  2. In earlier PR for this issue several approaches were checked and we came up to this solution.
  3. Fixed
  4. Fixed
  5. I'm using the common pattern used in say u256, b512 or exponentiation test. Though I agree that unit tests should be do instead, as soon as they become avaliable.

rostyslavtyshko avatar Sep 02 '22 08:09 rostyslavtyshko

Is this ready for final review?

simonr0204 avatar Sep 06 '22 12:09 simonr0204

All the E2E tests need a test.toml and json_abi_oracle.json (unless you want to set validate_abi to false in test.toml. See https://github.com/FuelLabs/sway/tree/master/test/src/e2e_vm_tests/test_programs/should_pass/stdlib/assert_test for an example.

mohammadfawaz avatar Sep 07 '22 14:09 mohammadfawaz

@tyshkor don't forget to ping for final review If you think this is ready.

simonr0204 avatar Sep 18 '22 21:09 simonr0204