Re: [PATCH] dinput: mouse.c: extracted the warping code into a function, got rid from goto's in alloc_device, and made the code more readable. (no change in functionality)
First of all your patch is wrapped. Second, do not send white space changes only. Same for formatting - do not change formatting only. If you can't read it, reformat for yourself, or get used to reading some one else's code.
What's wrong with gotos? If you don't like it doesn't mean everyone else can't use them. Don't change that part. Copying 10 lines of code into 10 places doesn't justify removing 10 lines of code.
You should probably check the patch requirements: http://winehq.org/site/sending_patches
Vitaliy.
Adam Rimon wrote:
dlls/dinput/mouse.c | 415 +++++++++++++++++++++++++++++---------------------- 1 files changed, 235 insertions(+), 180 deletions(-)
Using gotos is ok, when it's really needed. There is no need to use gotos here - it doesn't save source lines. Why are you against changing the format and fixing the mixed using of tabs and white spaces? Anyway, extracting the warping code into a function makes the warping code more maintainable.
On 12/14/2007 11:07 PM, Vitaliy Margolen wrote:
First of all your patch is wrapped. Second, do not send white space changes only. Same for formatting - do not change formatting only. If you can't read it, reformat for yourself, or get used to reading some one else's code.
What's wrong with gotos? If you don't like it doesn't mean everyone else can't use them. Don't change that part. Copying 10 lines of code into 10 places doesn't justify removing 10 lines of code.
You should probably check the patch requirements: http://winehq.org/site/sending_patches
Vitaliy.
Adam Rimon wrote:
dlls/dinput/mouse.c | 415 +++++++++++++++++++++++++++++---------------------- 1 files changed, 235 insertions(+), 180 deletions(-)
Please bottom-post on this list.
Why are you against changing the format and fixing the mixed using of tabs and white spaces?
That's a rule here. Leaving in extraneous patches will get the patch rejected. If you insist on changing whitespace, do it in a separate patch (which, by the way, will also likely get rejected.)
So, whether or not he (or I, or anyone except AJ) is against it, your patch will get rejected as-is. Follow Vitaliy's advice to increase the likelihood of it getting accepted. --Juan
There is nothing to maintain in that warping code. It needs to be removed you right. But you can do that only after x11drv can properly warp mouse itself (and keep it from leaving Wine window).
And for format only changes - read that link. It explains what type of changes you should not send.
Vitaliy.
Adam Rimon wrote:
Using gotos is ok, when it's really needed. There is no need to use gotos here - it doesn't save source lines. Why are you against changing the format and fixing the mixed using of tabs and white spaces? Anyway, extracting the warping code into a function makes the warping code more maintainable.
On 12/14/2007 11:07 PM, Vitaliy Margolen wrote:
First of all your patch is wrapped. Second, do not send white space changes only. Same for formatting - do not change formatting only. If you can't read it, reformat for yourself, or get used to reading some one else's code.
What's wrong with gotos? If you don't like it doesn't mean everyone else can't use them. Don't change that part. Copying 10 lines of code into 10 places doesn't justify removing 10 lines of code.
You should probably check the patch requirements: http://winehq.org/site/sending_patches
Vitaliy.
Adam Rimon wrote:
dlls/dinput/mouse.c | 415 +++++++++++++++++++++++++++++---------------------- 1 files changed, 235 insertions(+), 180 deletions(-)
Yeah, I read it. Can you explain to me the logic behind this decision (In general, not about this specific code)?
I think formatting a code is an important thing to do.
It makes the code easier to understand for programmers who have never seen the code before,
and makes it also more maintainable.
In this code, for example, the white spaces and tabs are mixed, the curly braces are sometimes right next to the line before
and sometimes in the next line, etc.
-Adam
On 12/15/2007 08:18 AM, Vitaliy Margolen wrote:
There is nothing to maintain in that warping code. It needs to be removed you right. But you can do that only after x11drv can properly warp mouse itself (and keep it from leaving Wine window).
And for format only changes - read that link. It explains what type of changes you should not send.
Vitaliy.
Adam Rimon wrote:
Using gotos is ok, when it's really needed. There is no need to use gotos here - it doesn't save source lines. Why are you against changing the format and fixing the mixed using of tabs and white spaces? Anyway, extracting the warping code into a function makes the warping code more maintainable.
On 12/14/2007 11:07 PM, Vitaliy Margolen wrote:
First of all your patch is wrapped. Second, do not send white space changes only. Same for formatting - do not change formatting only. If you can't read it, reformat for yourself, or get used to reading some one else's code.
What's wrong with gotos? If you don't like it doesn't mean everyone else can't use them. Don't change that part. Copying 10 lines of code into 10 places doesn't justify removing 10 lines of code.
You should probably check the patch requirements: http://winehq.org/site/sending_patches
Vitaliy.
Adam Rimon wrote:
dlls/dinput/mouse.c | 415 +++++++++++++++++++++++++++++---------------------- 1 files changed, 235 insertions(+), 180 deletions(-)
On Saturday 15 December 2007 10:01:44 Adam Rimon wrote:
Yeah, I read it. Can you explain to me the logic behind this decision (In general, not about this specific code)?
You mess up the output of e.g. git blame without ever changing the functionality of the code.
I think formatting a code is an important thing to do.
Yes, but it needs to be done initially, or when the code is touched, not for it's own sake.
It makes the code easier to understand for programmers who have never seen the code before,
If they want to read the code the way you think it should be read.
and makes it also more maintainable.
Agreed.
In this code, for example, the white spaces and tabs are mixed, the curly braces are sometimes right next to the line before and sometimes in the next line, etc.
I think there's no argument about the less than optimal formatting of the code you're talking about. Nevertheless, we're just telling you that it's very unlikely that this patch will be rejected.
Alexandre doesn't like patches like that, you'll have to fork or deal with it. People on this list decided to deal with it, so don't be surprised if that's the answer you get again and again.
Cheers, Kai
-
Adam Rimon -
Juan Lang -
Kai Blin -
Vitaliy Margolen