Hello
Would it be possible to get more of the guidelines written down on the http://wiki.winehq.org/Developers-Hints or http://wiki.winehq.org/SubmittingPatches pages, to get them set in stone, so-to-speak? And also somehow show which components have different guidelines and what are they? Right now, all the information seems to get reiterated to each new developer, causing a lots of overhead to the reviewers.
After being subscribed to this mailing list for a few months, I have noticed a pattern with new developers trying to submit patches: 1) first patches tend to receive lots of style related feedback 2) due to the big list of changes requested, the developers tend to miss a few with their resend and have to re-resend the patches again or due to the initial patches being too "out-of-style" the reviewers missed a few places in the first round and point them out in later round, causing the re-resend 3) sometimes the cycle goes on for quite a while
And to confuse things more: 1) sometimes different reviewers have different/conflicting opinions about style rules 2) some rules are component specific, mostly set by the maintainer 3) http://wiki.winehq.org/SubmittingPatches has only general rules 3.1) "Follow the style of surrounding code as much as possible, except when it is completely impractical to do so" - there have been many cases where patches get rejected because although it was done in the style of surrounding code, the component (or wine overall) has adopted new style rules, which are not evident in the surrounding code at all. 3.2) as a minor issue, the reference to Developer-Hints in the guidelines is not linked to http://wiki.winehq.org/Developers-Hints
Lots of work has already been done with the Developer-Hints page, but many issues still come up in the mailinglist that have not been mentioned there.
UINT vs unsigned int, int* foo vs int *foo, -1 vs ~0U, function parameter naming (Hungarian notation ? Ok for API function definitions based on MSDN doc or not?). Officially LONG use is suggested because long differs between 64bit linux and 64bit windows. Should the developers also BOOL FLOAT etc uppercase "safe" defines everywhere or not? true/false vs TRUE/FALSE?
It also seems that (at least in D3D related code) LP* usages are not allowed, LPWSTR => const WCHAR * should be used for parameters. But for example in kernel32/volume.c the LPWSTR is used in 10 places as function parameter, const WCHAR * is used in only one place as function parameter. Is the LP* rule overall rule, is it D3D specific? If overall, then why is it so widely used in all the kernel files? If D3D specific, how does the developer know it beforehand?
Suggestions, comments etc ?
Regards, Indrek Altpere
2015-02-13 15:15 GMT+01:00 Indrek Altpere efbiaiinzinz@hotmail.com:
Hello
Hi Indrek, first of all, thanks for the email. I'm not sure there is any "official" Wine style guideline or that I know what that is. Here's my view though.
Would it be possible to get more of the guidelines written down on the http://wiki.winehq.org/Developers-Hints or http://wiki.winehq.org/SubmittingPatches pages, to get them set in stone, so-to-speak? And also somehow show which components have different guidelines and what are they? Right now, all the information seems to get reiterated to each new developer, causing a lots of overhead to the reviewers.
Sure, updating the wiki seems like a great idea. Notice that everyone can update it so feel free to just go ahead and write down anything you think it would be useful.
After being subscribed to this mailing list for a few months, I have noticed a pattern with new developers trying to submit patches:
- first patches tend to receive lots of style related feedback
- due to the big list of changes requested, the developers tend to miss a
few with their resend and have to re-resend the patches again or due to the initial patches being too "out-of-style" the reviewers missed a few places in the first round and point them out in later round, causing the re-resend 3) sometimes the cycle goes on for quite a while
And to confuse things more:
- sometimes different reviewers have different/conflicting opinions about
style rules 2) some rules are component specific, mostly set by the maintainer
Those two points really go together. You're right, different reviewers (and consequently the components they are responsible of) tend to use different styles. I agree it might be confusing but it looks like using a single style for all of wine / reformatting all the nonconforming code is simply not going to happen so we have to stick with that. FWIW it's not uncommon for other open source projects to use different code styles in different components, for one reason or another.
- http://wiki.winehq.org/SubmittingPatches has only general rules
3.1) "Follow the style of surrounding code as much as possible, except when it is completely impractical to do so" - there have been many cases where patches get rejected because although it was done in the style of surrounding code, the component (or wine overall) has adopted new style rules, which are not evident in the surrounding code at all.
This is very true. I guess the actual "rule" is more like "follow the style of recent patches touching surrounding code / in the same DLL". As an example, there are chunks of wined3d code which are very old and use different styles. If you are going to send a wined3d patch touching one of those chunks you generally want to use the current style anyway. You generally shouldn't reformat code just to make it conform to the newer style, but you can / should reformat lines you're touching anyway. It might look funny on the edge between old and new code, in that case you are usually free to do some adjustments. You certainly want to use the new style for new functions, regardless of what the surrounding code looks like.
3.2) as a minor issue, the reference to Developer-Hints in the guidelines is not linked to http://wiki.winehq.org/Developers-Hints
Lots of work has already been done with the Developer-Hints page, but many issues still come up in the mailinglist that have not been mentioned there.
UINT vs unsigned int, int* foo vs int *foo, -1 vs ~0U, function parameter naming (Hungarian notation ? Ok for API function definitions based on MSDN doc or not?). Officially LONG use is suggested because long differs between 64bit linux and 64bit windows. Should the developers also BOOL FLOAT etc uppercase "safe" defines everywhere or not? true/false vs TRUE/FALSE?
AFAIK Hungarian notation is always best avoided. Of course when you need to preserve API compatibility (e.g. public structure fields) there is no way around it. I don't think anyone wants to see int* foo either. LONG vs long is not a suggestion but a requirement if it's something affecting the API, because of the 64-bit implementation differences (i.e. LLP64 vs LP64). About the other data types I guess we're not entirely consistent in general. We usually prefer to avoid the caps data types but just try to follow the preexisting conventions in that DLL and you should be fine. There are no lowercase true and false in C89 so you need the uppercase constants. No discussions here ;)
It also seems that (at least in D3D related code) LP* usages are not allowed, LPWSTR => const WCHAR * should be used for parameters. But for example in kernel32/volume.c the LPWSTR is used in 10 places as function parameter, const WCHAR * is used in only one place as function parameter. Is the LP* rule overall rule, is it D3D specific? If overall, then why is it so widely used in all the kernel files? If D3D specific, how does the developer know it beforehand?
LPFOO types are particularly frowned upon since they are obfuscating the pointers and tend not to do the expected thing with const. I haven't checked but I guess that they are used so much in kernel32 just because they are in older code.
Suggestions, comments etc ?
Regards, Indrek Altpere
Thanks again for bringing this up. Hopefully if I wrote something wrong / stupid someone is going to correct me. Also this kind of discussion is often a recipe for flamewars, so I put my fire-resistant armor on just in case :P
-
Indrek Altpere -
Matteo Bruni