On Tue, Oct 11, 2011 at 01:30:52PM -0700, Jason Gerecke wrote:
> On Tue, Oct 4, 2011 at 9:13 PM, Peter Hutterer <peter.hutte...@who-t.net>
> wrote:
> > On Tue, Sep 27, 2011 at 06:03:55PM -0700, Jason Gerecke wrote:
> >> + * @return Pointer to the requested data. Must be 'free()'-ed
> >> + */
> >> +static void* _get_property(Display *dpy, XDevice *dev, const char
> >> *prop_name,
> >> + int format, Atom type, unsigned long *items)
> >
> > this should better be nitems to avoid confusion between the actual items and
> > the _number_ of items. also, I'd prefer if you return char* instead of
> > void*.
> > also, no real need for unsigned long here, might as well use int or uint.
> >
> I'm still too new to C to understand all the nuances, so I must ask --
> why should char* be preferred over void* in this instance? I've seen
> both used in situations like this, but it seems that void* is the more
> correct type (mostly based on the observation that functions like
> malloc, memset, and memcmp all use void* when the type cannot be
> known).
I prefer using char* for buffers. Yes, you can use void* especially if you
expect the buffer to be in multiple formats (like properties can be) but
having a char means you have your base format that can easier be printed,
debugged and handled.
There's nothing wrong with using void*, other than that you _always_ have to
typecast when you want to use it. char* you only need to typecast for other
formats. In this case, you'll probably notice that XGetDeviceProperty
returns a unsigned char* too.
(uchar is actually preferrable over char for buffers too)
> As for the unsigned long, that's the type I get back from
> XGetDeviceProperty -- switching to int and uint gives us nothing but
> more compiler warnings :)
just typecast them. if we end up using more than INT_MAX nitems in a
property, we've probably done something wrong.
the reason here is simple - the compiler warnings will always occur when
you're tying to go from long to int without typecasting. If you know you
never need the full range anyway, converting it to int closest to the source
is the best choice. Otherwise you'll spread a mix of ints and longs
throughout the program, typecasting them in random places. One place of
casting is much easier to debug if we ever find a bug with it.
>
> >> + }
> >> + else
> >> + {
> >> + data = tmp;
> >> + memcpy(&data[offset], read, read_items * format / 8);
> >> + XFree(read);
> >> + }
> >> + }
> >> + while (bytes_after > 0);
> >
> > the loop seems a bit excessive. just ask the server to fetch 1000 values or
> > so and we're covered for what we support, right?
> >
> I'd tend to agree with you, but the loop is here out of principle.
> Claiming "N should be enough" has always been a great way to get
> yourself into trouble. I'd rather the code be as correct as possible,
> even if it comes at the expense of some readability (though if you've
> got a more readable way to handle the loop, I'm all ears!).
the reason I write most code just to get 1000 values is twofold:
- writing loops to resize buffers etc. almost always introduce at least one
bug. requesting 1000 values and only getting 1 back is less error-prone.
- as above, if we ever have a property in the driver with 1000 values we've
probably done something very wrong.
so, is asking for 100 or 1000 values correct? no, but it's good enough and
sometimes that's all we need :)
> On Tue, Oct 4, 2011 at 9:19 PM, Peter Hutterer <peter.hutte...@who-t.net>
> wrote:
> >
> > actually, if you want to make this really useful, you could return a union
> > in the form
> >
> > union propdata {
> > char *c;
> > int *i;
> > float *f;
> > };
> >
> > and do the convertion to/fro the stupid property long format for 32-byte
> > properties. it would likely make the callers more clear too. see xinput's
> > src/property.c for an example.
> >
> Just implemented this and I'm not really sure if it helps with
> readability. If we know the correct type for the pointer (as is the
> case at most call sites), it makes things more difficult to read. On
> the other hand, when we don't know the correct type (like in
> 'get_param'), things are easier to read. I can post the patch with the
> return type switched to propdata if you want to compare them.
I agree that it isn't necessarily clearer to read but I noticed it reduces
the number of bugs and improves debugability, _especially_ with the screwed
64-bit handling of the property API (32 bit values as longs).
Bugs that are easily introduced when switching buffer formats are e.g.
long data = (long*)buffer[1];
did you mean (long*)(buffer[1]) or ((long*)buffer[1])? with the union, this
would be
long data = buffer.l[1];
Much more obvious and less error-prone.
Cheers,
Peter
------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure contains a
definitive record of customers, application performance, security
threats, fraudulent activity and more. Splunk takes this data and makes
sense of it. Business sense. IT sense. Common sense.
http://p.sf.net/sfu/splunk-d2d-oct
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
