Eliminating casts

Development archive
1

Hey,

In general, I'm not happy with casts when they are not really needed. A cast takes away an opportunity for the compiler to check things. Sometimes a cast is there to get rid of "const". That's not good. Sometimes a cast is there to get rid of "volatile". That's not good either.

Suppose I make a Pull Request to eliminate casts, would that be picked up? Who would pick them up? Who decides if the PR is valid? What the general opinion about these sorts of PRs?

If you want examples, here are some, from various drivers.

----------------------------8X------------8X------------------- void at86rf2xx_tx_exec(const at86rf2xx_t *dev) { netdev_t *netdev = (netdev_t *)dev; ----------------------------8X------------8X-------------------

----------------------------8X------------8X------------------- void kw2xrf_setup(kw2xrf_t *dev, const kw2xrf_params_t *params) { netdev_t *netdev = (netdev_t *)dev; ----------------------------8X------------8X-------------------

----------------------------8X------------8X------------------- static void _isr(netdev_t *netdev) { ethos_t *dev = (ethos_t *) netdev; dev->netdev.event_callback((netdev_t*) dev, NETDEV_EVENT_RX_COMPLETE); ----------------------------8X------------8X------------------- Cast flipflop.

----------------------------8X------------8X------------------- void tcs37727_read(const tcs37727_t *dev, tcs37727_data_t *data) { ... tcs37727_trim_gain((tcs37727_t *)dev, tmpc); ----------------------------8X------------8X------------------- Here tcs37727_trim_gain is actually modifying the const object. Bad.

----------------------------8X------------8X------------------- return cc2420_init((cc2420_t *)dev); ----------------------------8X------------8X------------------- In this case, dev is already of that type.

In the Coding Conventions I like to see something written about casts. I'm not sure exactly about the wording. Here is an attempt.

Casts * Try to avoid casts (a bit vague, but it should get people's attention) * Introduce helper variables to avoid multiple casts within a function * Don't cast a const pointer into a non-const pointer without an explanation in a comment. * Don't cast a pointer to a volatile object dropping the volatile without an explanation in a comment. * ...

2

I agree with your arguments. Placing casts where they are not needed may introduce errors in a future refactoring where a variable changes type. If you want your changes merged as smoothly as possible you should open a separate PR for each module you modify. It will make the review much easier. Stuff like the cc2420 change is going to get merged right away pretty much since it’s obvious that there are no side effects. The tcs37727 change requires more thought for the correct change to make and may lead to some review comments.

Best regards, Joakim

3

Hi,

Suppose I make a Pull Request to eliminate casts, would that be picked up?

Always welcome! +1 on Joakim's hint to keep the PR's small.

void at86rf2xx_tx_exec(const at86rf2xx_t *dev) { netdev_t *netdev = (netdev_t *)dev;

What would be the way to go here? 'netdev_t netdev = &dev->netdev;'?

In this case, it won't work. Would need to recurse into netdev_ieee802154_t, like 'netdev_t *netdev = &dev->netdev.netdev;'.

That might be less error.prone, but more confusing. Unfortunately C is not much help here.

Casts * Try to avoid casts (a bit vague, but it should get people's attention) * Introduce helper variables to avoid multiple casts within a function * Don't cast a const pointer into a non-const pointer without an explanation in a comment. * Don't cast a pointer to a volatile object dropping the volatile without an explanation in a comment. * ...

* use dereferenced superclass field instead of "blind" cast

(the netdev case above).

Kaspar

4

Hi,

Suppose I make a Pull Request to eliminate casts, would that be picked up?

Always welcome! +1 on Joakim's hint to keep the PR's small.

Sure

void at86rf2xx_tx_exec(const at86rf2xx_t *dev) { netdev_t *netdev = (netdev_t *)dev;

What would be the way to go here? 'netdev_t netdev = &dev->netdev;'?

Yes, please. It's already done in several drivers.

In this case, it won't work. Would need to recurse into netdev_ieee802154_t, like 'netdev_t *netdev = &dev->netdev.netdev;'.

That might be less error.prone, but more confusing. Unfortunately C is not much help here.

Is it more confusing? I don't think so. Personally, I prefer the &dev->netdev.netdev above the (netdev_t *)dev.

5

A controversial idea is to change the implementation to C++ and make the driver a proper class with inheritance and utilize polymorphism with dynamic casts.

/Joakim

6

PR per driver sounds good to me.

The required change in tcs37727 indeed involves a bit more. But I hope that everyone agrees that you cannot change a const object into a non-const object.

Take a look at the doxygen. The param[in] is misleading, the object will be modified. Ha, it even says so in the description.

/**

  • @brief Read sensor’s data