On Mon, Oct 12, 2009 at 2:18 PM, Charles Davis cdavis@mymail.mines.edu wrote:
Keith Muir wrote:
Charles Davis wrote:
Damn it, I keep forgetting to reply all.
Charles Davis wrote:
Vitaliy Margolen wrote:
Charles Davis wrote:
Hi,
My name is Charles Davis, but you all can call me "Chip".
I'm new to Wine development, and frankly, I'm kinda scared because I've heard and read that getting patches into Wine is very difficult. In fact, every time I got ready to send this first patch, I chickened out and couldn't do it. So, I want someone to review this patch to see if there's anything wrong with it.
Thanks for writing the patch.
Few things about your patch:
- Copyright 2008, 2009 Charles Davis
You need to add substantial amount of code to or be working for a long time on this file to add yourself to the copyright note. As for your contribution to the project, all authors are tracked and AUTHORS file updated automatically.
Oh. I see. But I have many more patches for this file.
- switch(fmt->Format)
- {
- case IOCTL_CDROM_CURRENT_POSITION:
- memset(&data->CurrentPosition, 0, sizeof(data->CurrentPosition));
- break;
- case IOCTL_CDROM_MEDIA_CATALOG:
- memset(&data->MediaCatalog, 0, sizeof(data->MediaCatalog));
- break;
- case IOCTL_CDROM_TRACK_ISRC:
- memset(&data->TrackIsrc, 0, sizeof(data->TrackIsrc));
- }
- /* We need IOCDAudioControl for IOCTL_CDROM_CURRENT_POSITION */
- if (fmt->Format == IOCTL_CDROM_CURRENT_POSITION)
- {
- ERR("This version of Mac OS X does not support IOCDAudioControl\n");
- return STATUS_NOT_SUPPORTED;
- }
This doesn't seems right. If it's not supported (aka error) you shouldn't be touching returned data. Also why are you zeroing return buffers? I don't see other implementations do it.
Good catch. I should have done the check first.
- case IOCTL_CDROM_MEDIA_CATALOG:
- memset(&ioc, 0, sizeof(ioc));
and
- case IOCTL_CDROM_TRACK_ISRC:
- memset(&ioc, 0, sizeof(ioc));
You shouldn't need to zero buffer you pass to system ioctl.
Good point.
- if((io = ioctl(fd, DKIOCCDREADMCN, &ioc.mcn)) == -1) break;
- if((io = ioctl(fd, DKIOCCDREADISRC, &ioc.isrc)) == -1) break;
Please keep formatting consistent with the rest of the function/file. Here you need to add space after "if".
You're right. I read about that, but that must have slipped by me.
Vitaliy.
Thanks for your input.
Chip
The Julliard rank is something worth explaining. Wine developers have long wondered exactly *HOW* Alexandre determines whether or not a specific patch gets committed. The process is something as follows:
* Does the patch use C++ style comments? Yes: REJECT * Does the patch change coding style from the surrounding code? Yes: REJECT * Is the code obviously wrong in any way? Yes: REJECT * Does it take more than a few seconds to see that the code is correct? If today is a busy day: IGNORE * Is the author on the s**t list? Yes: REJECT * Are the planets aligned in just the right way and none of the above reject? Yes : ACCEPT
And of course now we need some sort of measurement for inclusion in the crap list. For every 'good' patch set an author moves further off the list, for every bad patch the author's reputation is diminished and put further onto the list. Most Wine developers have come to realize that that the Julliard Rank is very similar to Google's PageRank algorithm: everybody who cares understands how it is 'supposed' to work but could never replicate it in a million years.
OK, I've sent a better version of my patch to wine-patches. Now we'll see what AJ thinks of it.
Apparently he liked it, it was committed today http://source.winehq.org/git/wine.git/?a=commitdiff;h=c7992a8d260e0ce4073f85...
Well done, now get some more patches in ;-)