5 ms·

How specifically did you find the Rust version harder to grasp?

diegocg 5y ago
Rust syntax isn't particularly friendly.
magicalhippo 5y ago
I'm not well versed in Rust, so that's obviously a factor. However the Rust version seems to have more line noise, like all the ".ok_or(Error::ENXIO)?;" bits that are sprinkled all over. Also compare the pl061_probe function and it's Rust equivalent. The C version is fairly straight forward and clean in terms of syntax, while the Rust function looks very complex in comparison. Also if you compare pl061_irq_ack with the Rust equivalent, suddenly the Rust version has an if statement in there while the C does not. What happens when the if statement is not taken?
- xondono 5y ago
  In some part the problem here is that Rust is making error handling explicit, while the C driver has all those error handling covered in other files. For example, in the C version: struct pl061 *pl061 = gpiochip_get_data(gc); What happens if there's no chip? you should have a NULL check, but it's omitted because error handling is done outside of this file. The same is true for the additional if statement on the IRQ. In the end is a side effect of this being an example, because it should be possible to refactor the code where error handling is simply done through the ? operator. IMO that's a better way to do it than the C alternative, because right now there's no way to know if those struct pointers have proper error handling or not without leaving this file and hunting for each struct and function.
- moltonel3x 5y ago
  "opt.ok_or(err)?" is a pretty neat way of returning early with the error "err" if opt is empty. This is pretty idiomatic Rust you should get familiar with before comparing the readability with a language you know better. I see a few places where Rust is definitely more readable, for example for offset in 0..PL061_GPIO_NR { if inner.csave_regs.gpio_dir & bit(offset) != 0 { vs for (offset = 0; offset < PL061_GPIO_NR; offset++) { if (pl061->csave_regs.gpio_dir & (BIT(offset))) OTOH, the next line is a real mouthful in Rust compared to C: if let Ok(v) = <Self as gpio::Chip>::get(data, offset.into()) { inner.csave_regs.gpio_data |= (v as u8) << offset; } vs pl061->csave_regs.gpio_data |= pl061_get_value(&pl061->gc, offset) << offset; * "<Self as gpio::Chip>::" is the price Rust pays for having multiple possible "get()" for the same data, but it could be hidden away in a wrapper if needed. * ".into()" might be due to a young not-yet-ergonomic API * The last difference comes from the API difference where Rust's get() actually tells you if it could get that integer instead of (presumably) returning the integer 0. The C API is arguably a footgun, justifying Rust's slightly wordier syntax. For a more subjective example, I find "pl061.base.readb(GPIODIR)" more readable than "readb(pl061->base + GPIODIR)". Bonus: the offset argument can be enum-typed, avoiding the footgun of reading from an invalid offset. Going back to the probe functions, I find them hard to compare as I do not know how the hardware works. If that "device::Data::new()" is equivalent to the many lines of init in the C version, it looks less footguny. In the same vein, "Ref::try_new_and_init()" and "data.registrations()" look like they are giving the developer more guarantees. There's a trend there : some Rust APIs may be wordier but still easier to read/review, because they uphold more invariants, which reduces the reviewer's cognitive load. Concerning the ack() functions I'm not sure. It seems that Rust is checking that it has proper access to the data ressource while C doesn't. I would turn the question around : How are you sure that "gpiochip_get_data(irq_data_get_irq_chip_data(...))" never fails and can be safely dereferrenced ?
  - magicalhippo 5y ago
    > If that "device::Data::new()" is equivalent to the many lines of init in the C version, it looks less footguny. The Rust version is probably more robust, I would hope and assume so. However it looks considerably more complex, I think even if I knew Rust I'd have a harder time parsing it and trying to figure out what it tries to do, compared to the C version. So maybe we could say the Rust version is harder to parse but easier to reason about once parsed. > How are you sure that "gpiochip_get_data(irq_data_get_irq_chip_data(...))" never fails and can be safely dereferrenced ? Fair point, it should have a check there. Which makes this comparison a bit harder, given that the code is so different.
    - yazaddaruvala 5y ago
      > So maybe we could say the Rust version is harder to parse but easier to reason about once parsed. I feel this is a matter of familiarity. For me the Rust version is easier to parse and reason about. Entirely because of familiarity.
- deleted 5y ago
  [deleted]

Rust syntax isn't particularly friendly.

I'm not well versed in Rust, so that's obviously a factor. However the Rust version seems to have more line noise, like all the ".ok_or(Error::ENXIO)?;" bits that are sprinkled all over. Also compare the pl061_probe function and it's Rust equivalent. The C version is fairly straight forward and clean in terms of syntax, while the Rust function looks very complex in comparison. Also if you compare pl061_irq_ack with the Rust equivalent, suddenly the Rust version has an if statement in there while the C does not. What happens when the if statement is not taken?

xondono 5y ago
In some part the problem here is that Rust is making error handling explicit, while the C driver has all those error handling covered in other files. For example, in the C version: struct pl061 *pl061 = gpiochip_get_data(gc); What happens if there's no chip? you should have a NULL check, but it's omitted because error handling is done outside of this file. The same is true for the additional if statement on the IRQ. In the end is a side effect of this being an example, because it should be possible to refactor the code where error handling is simply done through the ? operator. IMO that's a better way to do it than the C alternative, because right now there's no way to know if those struct pointers have proper error handling or not without leaving this file and hunting for each struct and function.
moltonel3x 5y ago
"opt.ok_or(err)?" is a pretty neat way of returning early with the error "err" if opt is empty. This is pretty idiomatic Rust you should get familiar with before comparing the readability with a language you know better. I see a few places where Rust is definitely more readable, for example for offset in 0..PL061_GPIO_NR { if inner.csave_regs.gpio_dir & bit(offset) != 0 { vs for (offset = 0; offset < PL061_GPIO_NR; offset++) { if (pl061->csave_regs.gpio_dir & (BIT(offset))) OTOH, the next line is a real mouthful in Rust compared to C: if let Ok(v) = <Self as gpio::Chip>::get(data, offset.into()) { inner.csave_regs.gpio_data |= (v as u8) << offset; } vs pl061->csave_regs.gpio_data |= pl061_get_value(&pl061->gc, offset) << offset; * "<Self as gpio::Chip>::" is the price Rust pays for having multiple possible "get()" for the same data, but it could be hidden away in a wrapper if needed. * ".into()" might be due to a young not-yet-ergonomic API * The last difference comes from the API difference where Rust's get() actually tells you if it could get that integer instead of (presumably) returning the integer 0. The C API is arguably a footgun, justifying Rust's slightly wordier syntax. For a more subjective example, I find "pl061.base.readb(GPIODIR)" more readable than "readb(pl061->base + GPIODIR)". Bonus: the offset argument can be enum-typed, avoiding the footgun of reading from an invalid offset. Going back to the probe functions, I find them hard to compare as I do not know how the hardware works. If that "device::Data::new()" is equivalent to the many lines of init in the C version, it looks less footguny. In the same vein, "Ref::try_new_and_init()" and "data.registrations()" look like they are giving the developer more guarantees. There's a trend there : some Rust APIs may be wordier but still easier to read/review, because they uphold more invariants, which reduces the reviewer's cognitive load. Concerning the ack() functions I'm not sure. It seems that Rust is checking that it has proper access to the data ressource while C doesn't. I would turn the question around : How are you sure that "gpiochip_get_data(irq_data_get_irq_chip_data(...))" never fails and can be safely dereferrenced ?
- magicalhippo 5y ago
  > If that "device::Data::new()" is equivalent to the many lines of init in the C version, it looks less footguny. The Rust version is probably more robust, I would hope and assume so. However it looks considerably more complex, I think even if I knew Rust I'd have a harder time parsing it and trying to figure out what it tries to do, compared to the C version. So maybe we could say the Rust version is harder to parse but easier to reason about once parsed. > How are you sure that "gpiochip_get_data(irq_data_get_irq_chip_data(...))" never fails and can be safely dereferrenced ? Fair point, it should have a check there. Which makes this comparison a bit harder, given that the code is so different.
  - yazaddaruvala 5y ago
    > So maybe we could say the Rust version is harder to parse but easier to reason about once parsed. I feel this is a matter of familiarity. For me the Rust version is easier to parse and reason about. Entirely because of familiarity.
deleted 5y ago
[deleted]