Fixes #982

[Dirbaio]

Fix looks good!

Could you add to CI a 16bit target that would've triggered the errors, so that CI catches similar busg in the future?

[d2weber]

I could try to reproduce with the msp430-none-elf target (so we wouldn't need a separate target.json). But there is no std. Currently in ci.sh test all feature combinations are tested. One option would be maintaining a separate set of "no-std" feature combinations. Or I check if it'd be enough to run clippy with the msp430 target.

[d2weber]

Just checked, it's reproducible with the msp430-none-elf target. But the standard library cannot be compiled with 16 bit pointer-width, so only no-std feature sets can be tested. I'd need help figuring out, which feature sets make sense here.

Then I could add something like this in ci.sh

clippy() {
    rustup +nightly component add rust-src
    cargo +nightly clippy -Z build-std=core,alloc --target msp430-none-elf --no-default-features --features="socket-tcp proto-ipv4 medium-ethernet"
}

By the way, I'm getting lots of warnings with cargo +beta clippy.

[Dirbaio]

the errors are caught by rustc, not clippy, right? If so, just doing a cargo build for msp430 should be enough?

For the features i'd just do one build, enabling all features that do work on nostd and ignoring the others.

[d2weber]

Yes you are right, its build.

[d2weber]

I can't find the location where this error originates. Line numbers seem to be off. Maybe there is some macro magic going on? I also can't reproduce this error locally, no idea what's going on:

error: this arithmetic operation will overflow
   --> src/socket/tcp.rs:699:67
    |
699 |         Some(cmp::min(last_win_adjusted >> self.remote_win_shift, (1 << 16) - 1) as u16)
    |                                                                   ^^^^^^^^^ attempt to shift left by `16_i32`, which would overflow
    |
    = note: `#[deny(arithmetic_overflow)]` on by default

[thvdveld]

@d2weber I think actions/checkout was drunk and used an old commit. I opened a PR to update to the latest version of actions/checkout. You might want to rebase on main once that PR is merged.

[thvdveld]

I don't understand why CI is using the wrong source code. All jobs are checking out 19464653, which looks fine, but then when the tests are running, the source doesn't look like what's in 19464653.

Never mind, you actually have (1 << 16) - 1 on line 699 in src/socket/tcp.rs.

[d2weber]

~I don't understand why CI is using the wrong source code. All jobs are checking out 1946465, which looks fine, but then when the tests are running, the source doesn't look like what's in 1946465.~

Never mind, you actually have (1 << 16) - 1 on line 699 in src/socket/tcp.rs.

No idea how I missed that. I have to check later, what went wrong, maybe my editor didn't reload the file after the rebase...

[thvdveld]

I didn't catch it at first sight either! Thanks for fixing it.