Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix 064_builtins: @addWithOverflow() explanation #94

Merged
merged 1 commit into from Apr 11, 2022

Conversation

DerTee
Copy link
Contributor

@DerTee DerTee commented Apr 3, 2022

The last two examples do not overflow, because the
result is small enough. This was probably just a typing
error in the original explanation.

The last two examples do not overflow, because the
result is small enough. This was probably just a typing
error in the original explanation.
@ratfactor ratfactor merged commit 738cb67 into ratfactor:main Apr 11, 2022
@ratfactor
Copy link
Owner

@DerTee Thanks! Definitely an oversight.

@DerTee DerTee deleted the patch-1 branch April 13, 2022 03:24
@ziyi-yan
Copy link
Contributor

Is it really an oversight? I think the several lines show the process of adding 0101 to 1010 one by one. The two "YES!" show the overflow status of last two steps. At that time, the overflow has already happened and "Overflowed?" is "YES!".

@DerTee
Copy link
Contributor Author

DerTee commented Apr 18, 2022

@ziyi-yan I thought about that too, but in the context of the builtin @addWithOverflow function that does not make sense, because it returns a result per operation, not for several operations. Also afaik the flags of CPUs are also set for the last operation. Also @ratfactor wrote the original text and confirmed it was an oversight, so I think everything is alright :)

@ratfactor
Copy link
Owner

@ziyi-yan and @DerTee Yeah, thanks for the additional thoughts on this. I really wish I knew what I was originally intending with those lines. It's strange. I wonder if it might be even better just to remove them?

@DerTee
Copy link
Contributor Author

DerTee commented Apr 21, 2022

@ratfactor I actually think that incrementing a value up to and over the point where it overflows is a good idea. Removing the two lines that come after the overflow would help to not confuse people about the builtins exercise, but they might also come out with a false understanding of overflow.

The crux is that counting up a variable and exceeding its capacity overflows only in one single @addWithOverflow operation, but afterwards the variable stays overflowed even though the subsequent counting operations do not overflow. So there's a difference between a overflowing operation and an overflowed value. I don't know if it's worth teaching these finer details within the context of teaching Zig's builtins, though. Maybe introducing @as in this first exercise or one of the other builtins would be better?

Anyway, here's my shot at trying to make the @addWithOverflow part clearer:

    // Let's try it with a tiny 4-bit integer size to make it clear:
    var a: u4 = 0b1101;
    const b: u4 = 0b0001;
    const count: u4 = 5;
    var my_result: u4 = undefined;
    var overflowed: bool = undefined;

    // Let's count up and see when we get an overflow in a nice table:
    print("  a  |  b   | result | overflowed?\n", .{});
    print("----------------------------------\n", .{});
    var i: u4 = 0;
    while (i < count) : (i += 1) {
        overflowed = @addWithOverflow(u4, a, b, &my_result);

        // Check out our fancy formatting! b:0>4 means, "print
        // as a binary number, zero-pad right-aligned four digits."
        print("{b:0>4} + {b:0>4} =  {b:0>4}  | {}\n", .{ a, b, my_result, overflowed });

        // To make counting work, we store the result in variable a.
        a = my_result;
    }

    // Now we check the difference between the result we get with overflows
    // and the result we get without an overflow
    const expected_a: u8 = 0b00010010;
    print("Our overflowed 'a' is {b:0>4}. Without overflow it would be {b:0>8}.\n", .{ a, expected_a });

    print("Furthermore, ", .{});

The full output would then look like this:

  a  |  b   | result | overflowed?
----------------------------------
1101 + 0001 =  1110  | false
1110 + 0001 =  1111  | false
1111 + 0001 =  0000  | true
0000 + 0001 =  0001  | false
0001 + 0001 =  0010  | false
Our overflowed 'a' is 0010. Without overflow it would be 00010010.
Furthermore, 11110000 backwards is 00001111.

I'm not sure if it's really better, but I'm trying to be as constructive as I can and at least propose some solution.

@ratfactor
Copy link
Owner

@DerTee Yeah, I see where you're going with that. It would be really nice to see the moment where the value overflowed!

I've tried to keep the correct output of Ziglings exercises short to make them easy to check for correctness (at least partially inspired by the single numeric answers to exercises in https://projecteuler.net/). Originally, I was shooting for single line output, but that was too limiting.

I'll have to think about this. The table of output would be really nice.

@ratfactor
Copy link
Owner

Still thinking about this. I love the table idea, but would also like to avoid such a large "answer" output. Is there some way to display this same information in a compact way without losing too much clarity?

@DerTee
Copy link
Contributor Author

DerTee commented May 1, 2022

@ratfactor Probably the best way is to not generate the table in code, but simply put the output table in the comments, just like you did originally. I have a draft we can have a look at. I'll push it into this PR as soon as I figure out the github stuff (I screwed something up locally).

EDIT: can't figure out how to push into this PR. Maybe that's because it's closed, no idea. My draft for changes is here: DerTee@c94382b
I can of course create a new PR, but that disconnects the discussion.

@DerTee DerTee restored the patch-1 branch May 1, 2022 23:38
@ratfactor
Copy link
Owner

@DerTee Sorry for the delay getting back to you on this. I like what you've got! Yeah, do please create a new PR. I'm sure we can link the two somehow by mentioned this one in that one.

vamega pushed a commit to vamega/ziglings that referenced this pull request Jul 25, 2023
fix  064_builtins: @addWithOverflow() explanation
rej696 pushed a commit to rej696/ziglings that referenced this pull request May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants