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
*
* Besides an Autogain routine is called. If a maximum or minimum threshold
* value of the channel clear is reached, then the gain will be changed
* correspond to max or min threshold.
*
* @param[in] dev device descriptor of sensor
* @param[out] data device sensor data, MUST not be NULL
*/
void tcs37727_read(const tcs37727_t *dev, tcs37727_data_t *data);
On 26-12-18 23:29, Joakim Nohlgård wrote:
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
Den ons 26 dec. 2018 23:17 skrev Kees Bakker <k...@sodaq.com
<mailto:k...@sodaq.com>>:
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.
* ...
--
Kees
_______________________________________________
devel mailing list
devel@riot-os.org <mailto:devel@riot-os.org>
https://lists.riot-os.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel
