-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Hi,
After recently helping two contributors (the Pipelight guys and Martin Storsjö) through the maze of the Wine code style I think it's best to lift the ban against style-only patches and unify the code style once and for all.
The basic reasoning is that from my point of view the lack of clarity wrt the code style and lack of automatic enforceability is causing more issues than the problems style-only patches cause with git blame. Finding a style-only change with git blame can be handled by running git blame again. Frustrating contributors on the other hand has no workaround.
So I propose to agree on a code style for the entire project, fixing it up preferably with an automated tool and in the future enforcing the style with checks in the Testbot.
Opinions?
Wrt the code style I propose the style that is used in wineserver. It is the same one as the new style in the d3d code except for single-line if conditions.
Cheers, Stefan
On 02/13/14 14:09, Stefan Dösinger wrote:
Hi,
After recently helping two contributors (the Pipelight guys and Martin Storsjö) through the maze of the Wine code style I think it's best to lift the ban against style-only patches and unify the code style once and for all.
The basic reasoning is that from my point of view the lack of clarity wrt the code style and lack of automatic enforceability is causing more issues than the problems style-only patches cause with git blame. Finding a style-only change with git blame can be handled by running git blame again. Frustrating contributors on the other hand has no workaround.
So I propose to agree on a code style for the entire project, fixing it up preferably with an automated tool and in the future enforcing the style with checks in the Testbot.
Opinions?
Well, another solution is to be less picky about coding style during reviews. You gave me a perfect example just after writing this mail: http://www.winehq.org/pipermail/wine-devel/2014-February/102864.html I'm mostly talking about the second comment. Seriously? Is that worth a resend?
I know it's often tempting to ask for code style change and there is often no clear answer if it's worth or not. However, my feeling is that d3d reviewers tend to be way more picky about that than others and maybe we could solve most of the problems on reviewers' side. Note that I'm not saying that no code style comments should be made, but those could be limited to cases that really make sense.
Wrt the code style I propose the style that is used in wineserver. It is the same one as the new style in the d3d code except for single-line if conditions.
There is another problem with coding style than git blame...
No, I don't want this style. I hate new lines for each opening bracket like:
if(x) { x = 0; y = 1; }
This makes the code almost 2 times longer than it needs to be for no good reason. If you need more space to make code cleaner, simply add a blank line where they are useful, not for every bracket.
Let's use jscript style ;)
Cheers, Jacek
On 13 February 2014 14:57, Jacek Caban jacek@codeweavers.com wrote:
often no clear answer if it's worth or not. However, my feeling is that d3d reviewers tend to be way more picky about that than others and maybe we could solve most of the problems on reviewers' side.
I'm not quite sure where you got that impression, in the general case anyway. Personally, I'll point out style issues if the patch is going to need changes anyway, but I'm not going to ask somebody to redo a patch for style reasons alone, unless it's wildly inconsistent.
For the record, I think this thread is a waste of time, and would otherwise have ignored it.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2014-02-13 14:57, schrieb Jacek Caban:
Well, another solution is to be less picky about coding style during reviews. You gave me a perfect example just after writing this mail:
http://www.winehq.org/pipermail/wine-devel/2014-February/102864.html
I'm mostly talking about the second comment. Seriously? Is that worth a resend?
Maybe, or maybe not. The basic issue still stands. New contributors will look at the existing code, and if its inconsistent how are they supposed to write code that's consistent in itself. That's not a question of pickiness but approach to the problem.
Take a look at e.g. http://www.winehq.org/pipermail/wine-patches/2014-February/130297.html . It has plenty of inconsistencies, and none of them are the contributor's fault. Should we ignore that as well? If so, should we allow the codebase to look like that?
My impression was that the answers to that were no and no, with the idea that the problem will be fixed by unifying the style together with other, functional changes. That way we end up with picky reviews and IMO that approach is bad.
Martin, Michael or Sebastian: If you are reading this thread maybe you can share your thoughts from the point of view of someone who isn't a long-term contributor.
On Thu, 13 Feb 2014, Stefan Dösinger wrote:
Martin, Michael or Sebastian: If you are reading this thread maybe you can share your thoughts from the point of view of someone who isn't a long-term contributor.
I'll try to stay out of the rest of this discussion, but I can at least agree that it's hard to get the coding style right when all the surrounding code is consistently (more or less) in another style, and the pointed out "good style" example files don't use all features (cs.c didn't have any switch cases).
// Martin
Am 13.02.2014 15:24, schrieb Stefan Dösinger:
Am 2014-02-13 14:57, schrieb Jacek Caban: Martin, Michael or Sebastian: If you are reading this thread maybe you can share your thoughts from the point of view of someone who isn't a long-term contributor.
I have to agree with Stefan, for someone new trying to contribute to the project its really confusing. Often not even inside a single DLL a unique style is used, and especially tests are often even a bit worse.
When someone wants to contribute, he might just think that there isn't any code style restriction at all. But even with the knowledge that there is one, its sometimes not very clear what the rules are exactly. Noone would start by creating statistics first, to figure out what the most commonly used style is.
To sum it up, I would also support the idea to refactor the whole code base.
Several methods (ordered from best to worst):
* refactor the code globally, such that every component uses the same style. This would be the easiest method for people to check their own code, since they can just use a checktool on the whole wine source.
* at least a single code style for each DLL/component individually, and someone who documents the used code style for every component if its not obvious (like a header field or as part of the spec file?).
* as a last option, but very complicated, and most likely not worth the effort: instead of refactoring Wine could provide some git precommit hook / other check tool, to test patches if they match the rules for a specific component
BTW: with "git blame -w" its not even necessary to run it twice ;)
Regards, Sebastian
On Thu, Feb 13, 2014 at 7:24 AM, Stefan Dösinger stefandoesinger@gmail.com wrote:
... Maybe, or maybe not. The basic issue still stands. New contributors will look at the existing code, and if its inconsistent how are they supposed to write code that's consistent in itself. That's not a question of pickiness but approach to the problem. ...
I'm not sure it's always clear what to do even the case of more experienced contributors, I've been taking a look at fixing a double unlock bug in RtlAcquireResourceExclusive ( http://source.winehq.org/source/dlls/ntdll/rtl.c#L157 ). And the style there indent-wise is so screwy that it's not clear what I should do in my patch. This one function currently has (level wise): 1) Four space indents 2) A tab indent + a space indent (in some cases just the tab) 3) A tab indent + a five space indent (alternatively, 13 spaces) 4) Two tabs + a space indent
So, what do I do? Do I just change line 190 to fix the bug and keep the two tab + a space indent? Do I fix just this line to conform to the standard since it's the only one I need to touch? Do I fix this one function because a lot of it doesn't conform to the standard? Personally, I would like to have a separate formatting patch to fix the function so that it's clear that I'm just fixing the one thing - but that's expressly prohibited under the current guidelines.
... My impression was that the answers to that were no and no, with the idea that the problem will be fixed by unifying the style together with other, functional changes. That way we end up with picky reviews and IMO that approach is bad. ...
I agree, Sebastian and I have been debating what to do with this function (and RtlAcquireResourceShared) for some time and style is the only thing keeping us from submitting the one-line fix for these. Honestly, it would be really great if we could run something like astyle on the whole codebase so that we were always working off of a "known good" style. If we could get that implemented then we could potentially think about verifying the patch style on submission and auto-rejecting patches that have style problems. Personally, I think that would be an amazing improvement to our submission process, but I have to use K&R style for work and I hate feeling like an idiot when I slip into the wrong style for a project.
Best, Erich
On 2/15/2014 10:32, Erich E. Hoover wrote:
On Thu, Feb 13, 2014 at 7:24 AM, Stefan Dösinger stefandoesinger@gmail.com wrote:
... Maybe, or maybe not. The basic issue still stands. New contributors will look at the existing code, and if its inconsistent how are they supposed to write code that's consistent in itself. That's not a question of pickiness but approach to the problem. ...
I'm not sure it's always clear what to do even the case of more experienced contributors, I've been taking a look at fixing a double unlock bug in RtlAcquireResourceExclusive ( http://source.winehq.org/source/dlls/ntdll/rtl.c#L157 ). And the style there indent-wise is so screwy that it's not clear what I should do in my patch. This one function currently has (level wise):
- Four space indents
- A tab indent + a space indent (in some cases just the tab)
- A tab indent + a five space indent (alternatively, 13 spaces)
- Two tabs + a space indent
So, what do I do?
Please send a fix that changes only what you need to change, why not?
Do I just change line 190 to fix the bug and keep the two tab + a space indent? Do I fix just this line to conform to the standard since it's the only one I need to touch? Do I fix this one function because a lot of it doesn't conform to the standard? Personally, I would like to have a separate formatting patch to fix the function so that it's clear that I'm just fixing the one thing - but that's expressly prohibited under the current guidelines.
... My impression was that the answers to that were no and no, with the idea that the problem will be fixed by unifying the style together with other, functional changes. That way we end up with picky reviews and IMO that approach is bad. ...
I agree, Sebastian and I have been debating what to do with this function (and RtlAcquireResourceShared) for some time and style is the only thing keeping us from submitting the one-line fix for these.
I don't see a problem here, if it's a one-line fix just submit it while keeping existing formatting of a changed line, what's a big deal? Yes, sometimes style is inconsistent, sometimes whitespace formatting differs from line to line but that kind of existing code "features" should never stop you from sending a fix. In a situation like that I usually change several lines before and after to use 4 spaces indentation.
Honestly, it would be really great if we could run something like astyle on the whole codebase so that we were always working off of a "known good" style. If we could get that implemented then we could potentially think about verifying the patch style on submission and auto-rejecting patches that have style problems. Personally, I think that would be an amazing improvement to our submission process,
I don't think it's ever a case that potential contributor decides not to do any work solely after looking at some code style issues and saying - oh, well, what a mess, I'm not going to touch it. What actually happens when people send patches (or attach them to a bug and then disappear) is that patch is wrong in a first place, and code style issues are a second problem that's easy to fix comparing to functional part. Of course if all code uses same style we could simply avoid step 2 but that's not a blocking issue for new contributors.
Another thing is that some parts of wine have long time contributors, like wined3d, mshtml, msvcrt or audio bits. Why should we force their maintainers to some particular code style?
I personally can live with any decision made regarding reformatting everything, as long as it doesn't use some ugly mixed case names, variable names with type prefixes, LPLPLP stuff and such.
Anyway formatting issues never surprised me that much to stop fixing a real problem, but that's probably because I'm constantly jumping from one part to another, and from project to project on my daily job.
but I have to use K&R style for work and I hate feeling like an idiot when I slip into the wrong style for a project.
Best, Erich
On 15 February 2014 07:32, Erich E. Hoover erich.e.hoover@gmail.com wrote:
I'm not sure it's always clear what to do even the case of more experienced contributors, I've been taking a look at fixing a double unlock bug in RtlAcquireResourceExclusive ( http://source.winehq.org/source/dlls/ntdll/rtl.c#L157 ). And the style there indent-wise is so screwy that it's not clear what I should do in my patch. This one function currently has (level wise):
- Four space indents
- A tab indent + a space indent (in some cases just the tab)
- A tab indent + a five space indent (alternatively, 13 spaces)
- Two tabs + a space indent
So, what do I do? Do I just change line 190 to fix the bug and keep the two tab + a space indent? Do I fix just this line to conform to the standard since it's the only one I need to touch? Do I fix this one function because a lot of it doesn't conform to the standard?
Personally, in this specific case, I'd probably just change the single line, and replace the tabs with spaces there. In a module I'm more familiar with like wined3d I'd probably touch up slightly more. Except for reformatting the entire function, I doubt Alexandre would reject any of your approaches though, provided the patch is otherwise fine.
As for the original point of this thread, if Stefan spends considerable amounts of time explaining coding style to new contributors, perhaps it's worth the effort to maintain a page on the wiki, and point people to that instead.
On Sat, Feb 15, 2014 at 7:32 AM, Erich E. Hoover erich.e.hoover@gmail.com wrote:
... I agree, Sebastian and I have been debating what to do with this function (and RtlAcquireResourceShared) for some time and style is the only thing keeping us from submitting the one-line fix for these. Honestly, it would be really great if we could run something like astyle on the whole codebase so that we were always working off of a "known good" style.
Well, someone's "good style" is someone else's "bad style". For instance, I find "if(...)" constructs ugly, but some devs like it apparently, so if that's what's being used in the "surrounding code", it should be kept that way, for consistency.
What could/should be fixed, IMO, is code which is obviously broken/unclear, but limited to the enclosing block, but that should happen quite often.
Frédéric
Teegrins!
On 13/02/14 15:09, Stefan Dösinger wrote:
Hi,
After recently helping two contributors (the Pipelight guys and Martin Storsjö) through the maze of the Wine code style I think it's best to lift the ban against style-only patches and unify the code style once and for all.
The basic reasoning is that from my point of view the lack of clarity wrt the code style and lack of automatic enforceability is causing more issues than the problems style-only patches cause with git blame. Finding a style-only change with git blame can be handled by running git blame again. Frustrating contributors on the other hand has no workaround.
So I propose to agree on a code style for the entire project, fixing it up preferably with an automated tool and in the future enforcing the style with checks in the Testbot.
Opinions?
Wrt the code style I propose the style that is used in wineserver. It is the same one as the new style in the d3d code except for single-line if conditions.
Cheers, Stefan
I, for one, like this idea.
Not that my likes mean much (if anything), at least not yet, but I'm hoping one day I am able to contribute actual code to the project. As such, I have been delving more and more into the code of Wine, and I tend to learn from my surroundings, where inconsistency certainly isn't too helpful. It might also mean that I would further make it inconsistent by following whatever conventions happen to be around specific code (although that in itself could be considered consistent, in a way, though I am aware of changing code to follow a certain style being a thing while editing particular code for some other reason).
Either way, for what it's worth, I like the proposal.
Also, I have a more or less related (possibly silly) question as well, which is what pushed me to post here for the first time ever just now:
As I understand it, a whitespace is to be used after 'ifs' and such. Why is this not the case for TRACEs, ERRs, WARNs, and the likes as seen in the example below?
if (cs->state.textures[i] == prev) { TRACE("Texture is also bound to stage %u.\n", i); prev->sampler = i; break; }
Perhaps it's wrong for me to group those things together at all, but I guess it would just look better to me if there was a space after TRACE as well.
Just some thoughts!
Kind Regards (and many thanks), Chiitoo
(Soory for the top post, phone...)
I think the idea is to use spaces after keywords but not macro / function names. But I don't know what the exact logic and rationale is.
I'll write a wiki page about the wined3d code style the next time I review a patch with style issues or when I have some spare minutes and the necessary motivazion. Am 22.02.2014 19:45 schrieb "Chiitoo" escomk3@hotmail.com:
Teegrins!
On 13/02/14 15:09, Stefan Dösinger wrote:
Hi,
After recently helping two contributors (the Pipelight guys and Martin Storsjö) through the maze of the Wine code style I think it's best to lift the ban against style-only patches and unify the code style once and for all.
The basic reasoning is that from my point of view the lack of clarity wrt the code style and lack of automatic enforceability is causing more issues than the problems style-only patches cause with git blame. Finding a style-only change with git blame can be handled by running git blame again. Frustrating contributors on the other hand has no workaround.
So I propose to agree on a code style for the entire project, fixing it up preferably with an automated tool and in the future enforcing the style with checks in the Testbot.
Opinions?
Wrt the code style I propose the style that is used in wineserver. It is the same one as the new style in the d3d code except for single-line if conditions.
Cheers, Stefan
I, for one, like this idea.
Not that my likes mean much (if anything), at least not yet, but I'm hoping one day I am able to contribute actual code to the project. As such, I have been delving more and more into the code of Wine, and I tend to learn from my surroundings, where inconsistency certainly isn't too helpful. It might also mean that I would further make it inconsistent by following whatever conventions happen to be around specific code (although that in itself could be considered consistent, in a way, though I am aware of changing code to follow a certain style being a thing while editing particular code for some other reason).
Either way, for what it's worth, I like the proposal.
Also, I have a more or less related (possibly silly) question as well, which is what pushed me to post here for the first time ever just now:
As I understand it, a whitespace is to be used after 'ifs' and such. Why is this not the case for TRACEs, ERRs, WARNs, and the likes as seen in the example below?
if (cs->state.textures[i] == prev) { TRACE("Texture is also bound to stage %u.\n", i); prev->sampler = i; break; }
Perhaps it's wrong for me to group those things together at all, but I guess it would just look better to me if there was a space after TRACE as well.
Just some thoughts!
Kind Regards (and many thanks), Chiitoo
On 22/02/14 20:50, Stefan Dösinger wrote:
(Soory for the top post, phone...)
I think the idea is to use spaces after keywords but not macro / function names. But I don't know what the exact logic and rationale is.
I see. I guess that makes sense.
Thanks!
I'll write a wiki page about the wined3d code style the next time I review a patch with style issues or when I have some spare minutes and the necessary motivazion.
Good idea, and definitely something I will be looking forward to!
-
Chiitoo -
Erich E. Hoover -
Frédéric Delanoy -
Henri Verbeet -
Jacek Caban -
Martin Storsjö -
Nikolay Sivov -
Sebastian Lackner -
Stefan Dösinger