On 2013-03-05 11:45, Jerome Poncin wrote:
> The code with #ifdef DEBUG is also questionable, it will tend to bitrot
> since nobody will think to enable it.
>
> => I disagree with you, it could be helpful if somebody have a problem
> and want to debug quickly.
> But I can remove it if it's not standard to xenomai kernel.
#ifdef DEBUG is widely considered obsolete. If you have debug relevant
information, tracepoints are much better as they are runtime switchable.
But to look at a concrete example:
> +static ssize_t cifx_pci_read(struct rtdm_dev_context *context,
> + rtdm_user_info_t *user_info, void *buf,
> + size_t nbyte)
> +{
> + struct rtdm_device *info = (struct rtdm_device *)context->device;
> +
> + if (nbyte > sizeof(struct io_info)) {
> +#ifdef DEBUG
> + rtdm_printk
> + ("cifx rtdm driver error : data user size too big\n");
> +#endif /* DEBUG */
> +
> + return 0;
If it is an error, return the proper code (-EINVAL? Or -E2BIG?). That
will be visible without any driver instrumentation.
> + }
> +
> + /* Copy data information for userland */
> + if (rtdm_safe_copy_to_user(user_info, buf,
> + ((struct io_info *)info->device_data),
> + nbyte)) {
> +#ifdef DEBUG
> + rtdm_printk
> + ("cifx rtdm driver error : can't copy data from driver\n");
> +#endif /* DEBUG */
And this is a bug: return -EFAULT. Again, no need for driver
instrumentation.
> + }
> +
> + return nbyte;
> +}
...
>
> I think you are not convinced yet that "code is the best documentation"
>
> => Yes, you right it's because I worked long time on safety project (for
> me it's a habit) .
> Moreover I thunk it could be helpful in an open source project, if
> somebody want to more easily understand code.
Non-trivial code is better augmented with comments. However, commenting
on the obvious is like meaningless chatting. ;)
>
> The coding style is a bit indigest
>
> => I agree with you but that the price to be compliant with
> checkpatch.pl... limitation to 80 characters per line is difficult for me.
> If you saw some better thing to do, don't hesitate ;-) ! I tried to my
> best !
If you overrun the 80-char limit, it is usually a good sign that you
should introduce a function.
Jan
PS: Your mail client does funky marking of cited text. The web standard
is ">".
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
_______________________________________________
Xenomai mailing list
[email protected]
http://www.xenomai.org/mailman/listinfo/xenomai
