Kostya's Boring Codec World

As I’ve mentioned in the previous post, I’ve finally tried rust-clippy to see what issues and suggestions it will have on my code. The results are not disappointing if you take the tool name seriously.

The output (at least in my case) can be divided into four categories: actual bugs spotted, useful suggestions, (mostly) useless suggestions and counterproductive suggestions.

Let’s start with actual bugs spotted. It spotted two cases when condition has identical code in both branches. So I had to remove such useless condition in one case and fix the data source taken in another. Also an error on casting pointer to several-byte buffer as pointer to integer. Depending on stack alignment and CPU architecture this may result in a buggy load so better fix it (but then this issue will be mentioned again when we get to the third category).

Useful suggestions were much more plentiful. I started developing NihAV in Rust a bit more than two years ago and the language has changed quite a bit since then so some parts of code were a bit outdated (e.g. nowadays you can simply write Struct { field1, field2 } if you initialise it from field1 and field2 variables and have the same fields; previously you had to write Struct { field1: field1, field2: field2 }). Plus of course there are features I did not know about either because they were introduced later or I simply did not think about it being possible. Here are some examples I encountered:

• you can use vec![elem; size] to create vector of fixed size filled with element (previously I invoked Vec::with_capacity() and then vec.resize() which is more error-prone);
• now you can write ranges as start..=end instead of start..end+1 (IIRC this was still an unstable feature when I learned about it so no wonder I forgot about it);
• println!() is equivalent to println!("") (and guess who uses it for debugging);
• now instead of old way to increment pointer ptr.offset(size as isize) you can simply write ptr.add(size);
• there’s slice.swap(idx1, idx2) for the cases when you need to swap two values in an array;
• it also recognizes some standard constants and suggests to use those instead;
• it spotted useless casts too (e.g. when I cast u32 to u32);
• passing the reference to Vec when passing the reference to slice would work as good (and even save some MiNicycles on access); same for borrowing Box.

There were some stylistic suggestions that I found good (replacing some indexed loops with loops on iterators and copying slice with a call instead of loop) but mostly they belong to the next category, useless suggestions.

Yes, that’s where rust-clippy starts to live up to its name. Most of its suggestions were in the format “hey, you’re trying to use clear imperative programming to do the work that can be expressed with iterators in 10x text”. I understand that it’s more to the Rust style and it helps to prevent errors in some cases, but when it suggests to replace simple loop like for i in 3..7 { dst[i] = src[3 - i]; } with something that won’t fit onto single like I say it’s too much. Or how it suggested to change simple mem::transmute::<&mut $inttype, &mut [u8; $size]>(&mut buf) into &mut *(&mut buf as *mut $inttype as *mut [u8; $size]) (sure, it does the same but I find the former easier to comprehend).

Similar story with X as u32 versus u32::from(X). I understand it helps catching the cases where X changes type and cannot be converted to destination type losslessly any more. But it breaks formatting and makes it unnatural (and close to C++) in the sense that previously the code was read like “calculate X and cast it to type” and now it’s “convert to type from calculated X”. And by breaking formatting I mean that I have a lot of code in form

    let a = br.read(4)? as usize;
    let b = br.read(6)?;
    let c = br.read_bool()?;
    let d = br.read(3)? as u64;

and by using u64::from(br.read(3)?) I’d break nice tabular form for no obvious merit.
di
Another cases is when rust-clippy suggests refactoring conditions. When I write

    if is_intra {
        if block.coded { do a }
        else { do b }
    } else {
        if block.coded { do c }
        else { do d }
    }

I format it this way because it reflects the logic of the code and I do not want to make it into } else if block.coded { do c } else { do d }. Similarly if I don’t join let a; if foo { do stuff; a = val; } else { a = val2; } into single let a = if foo { ... val } else { val2 }; it’s because I have enough code in that condition that I don’t want to indent it even further (sure, make fun of me because I worked in text mode and still don’t like lines longer than 80-100 characters).

There’s more like “you should implement Default trait for each structure with new() method” and “you should write constants like 0x00_0001” and “enums should not have different prefix in the name, you should consider removing Mode from Mode16k, Mode5_5k and Mode8k” but let’s move to counterproductive suggestions already.

First issue is that rust-clippy interacts with macros quite poorly. For example I have a macros that expands 4-byte array into 32-bit tag and it suggests using u32::from(arr[0]) instead of (arr[0] as u32) but if you do that it will have an error instead of warning because it can’t check types in macro. Also here’s one of the WTFiest examples: I have a macro for filter implementation that takes coefficients as arguments and calculates the value. It has a code -$c1 * $src[$off + 1] and when $c1=-1 you have a warning “about –x is double negation”. Actually I remember that example from Feuer’s C. Puzzle Book. where #define NEG(x) -x NEG(-x) expanded to –x (and thus I put all macro arguments into parentheses) but I expected Rust to handle that correctly (otherwise this WTF goes to linter). In either case -(-1) is what I meant in that case and that’s how it works.

And now the most infuriating example that the tool treats like a warning or an error depending on situation.

As I write video decoders, I have a need to apply a filter or transform on some data. And IMO this form:

    dst[off - 2 * step] = a;
    dst[off - 1 * step] = b;
    dst[off + 0 * step] = c;
    dst[off + 1 * step] = d;

I think this conveys the meaning and looks much better than

    dst[off - 2 * step] = a;
    dst[off - step] = b
    dst[off] = c;
    dst[off + step] = d;

with whatever spaces you insert there (and it’s not possible to use iterators here quite often too). But rust-clippy treats that as a warning if you’re multiplying by one, adding zero or constant multiplied by zero and gives outright error on variable multiplied by zero or an expression like N - N (which happens when you have constant base and one of the offsets equal to it).

So my experience with this linter was mostly negative—all those neat things were out-weighted by the sheer amount of useless warnings (and I don’t like to add dozens of #[allow(clippy::useless_warning)] to lib.rs for many modules) and some stuff that would simply break code clarity for no obvious benefit. Overall, it’s a nice tool and though I got something useful from it, it may be a beginner in the language aka ordinary M$ Office user who would benefit the most from it and it will mostly hinder people writing advanced code unless you turn off a lot of warnings and even some errors.

This entry was posted on Saturday, May 18th, 2019 at 3:04 pm and is filed under NihAV, Rust, Useless Rants. You can follow any responses to this entry through the RSS 2.0 feed. Both comments and pings are currently closed.