On Thu, Feb 01, 2007 at 11:50:24AM -0800, Martin Rubli wrote:
> Hi Luigi,
>
> > attached are just a couple of trivial patches to
> > uvcvideo to suppress some compiler warnings - one
> > related to a missing const, one related to
> > pointer arithmetic done on a void *
>
> How did you trigger the warnings? The driver uses the standard kernel
> build system and on my machine no warnings show up ...
I should have said more - i am building this stuff on FreeBSD:
http://info.iet.unipi.it/~luigi/FreeBSD/linux_bsd_kld.html
so the set of compiler flags is different.
> > On passing - i notice that in a couple of structs
> > in uvcvideo.h you are used unnamed unions, which are
> > a gcc extension which is not guaranteed to be supported
> > in the future.
> >
> > I suppose that, besides convenience, the choice has been
> > inspired by a similar pattern in videodev2.h .
> >
> > Before the use gets too widespread, it might be a good
> > idea to use standard C and put a name to the union ?
> > The changes to the source now are relatively small,
> > and this would prevent the use
> > of unnamed unions in derived works, which would require
> > far more work to be fixed.
>
> If I'm not mistaken, GNU C (as opposed to ANSI C) is the official language
> used by the Linux kernel and there are quite a few other extensions that
> are consistently used throughout the kernel. I therefore don't think
> there's going to be a problem with that in the future. But If I'm missing
> something, please correct me!
one reason of concern is a note about future support that i misread
in http://gcc.gnu.org/onlinedocs/gcc/Unnamed-Fields.html:
... For example, this structure:
...
It is ambiguous which a is being referred to with `foo.a'.
Such constructs are not supported and must be avoided. In
the future, such constructs may be detected and treated as
compilation errors.
maybe the note on treating these as errors in the future
refers only to ambiguous constructs and not to all unnamed unions.
the other reason for concern is ease of porting to other systems -- things
like pragmas and attributes are easy to remap with a one time
#define, but unnamed extensions are a pain to handle because you
have to modify all references to the fields in the source.
and of course - there is the issue of code safety, because
1)this feature can cause ambiguous constructs, and 2) it doesn't
suggest the programmer that multiple items are sharing the same
memory location. The union name at least gives you some hints,
especially if you call it 'u' ...
> Laurent, can you check Luigi's patch anyway? Some of the changes make
> sense regardless of the above. Thanks.
cheers
luigi
> Cheers,
> Martin
>
>
> > diff -ubwr trunk/trunk/uvc_ctrl.c ./uvc_ctrl.c
> > --- trunk/trunk/uvc_ctrl.c Wed Jan 31 23:53:36 2007
> > +++ ./uvc_ctrl.c Wed Jan 31 23:29:59 2007
> > @@ -795,6 +795,7 @@
> > mutex_unlock(&uvc_driver.ctrl_mutex);
> > }
> > +static
> > void uvc_ctrl_add_mapping(struct uvc_control_mapping *mapping)
> > {
> > struct uvc_control_info *info;
> > diff -ubwr trunk/trunk/uvc_driver.c ./uvc_driver.c
> > --- trunk/trunk/uvc_driver.c Wed Jan 31 23:53:36 2007
> > +++ ./uvc_driver.c Wed Jan 31 21:36:15 2007
> > @@ -664,7 +664,7 @@
> > unit->type = VC_EXTENSION_UNIT;
> > memcpy(unit->extension.guidExtensionCode, &buffer[4], 16);
> > unit->extension.bNumControls = buffer[20];
> > - unit->extension.bNrInPins = le16_to_cpup((__le16*)&buffer[21]);
> > + unit->extension.bNrInPins = le16_to_cpup((const
> > __le16*)&buffer[21]);
> > unit->extension.baSourceID = (__u8*)unit + sizeof *unit;
> > memcpy(unit->extension.baSourceID, &buffer[22], p);
> > unit->extension.bControlSize = buffer[22+p];
> > diff -ubwr trunk/trunk/uvcvideo.h ./uvcvideo.h
> > --- trunk/trunk/uvcvideo.h Wed Jan 31 23:53:36 2007
> > +++ ./uvcvideo.h Wed Jan 31 23:23:50 2007
> > @@ -478,7 +478,7 @@
> > };
> > struct uvc_video_queue {
> > - void *mem;
> > + char *mem; /* we do pointer arithmetic here */
> > unsigned int streaming;
> > __u32 sequence;
> > __u8 last_fid;
> >
> >
> >
> > _______________________________________________
> > Linux-uvc-devel mailing list
> > [email protected]
> > https://lists.berlios.de/mailman/listinfo/linux-uvc-devel
>
>
_______________________________________________
Linux-uvc-devel mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/linux-uvc-devel
