"Kai Blin" kai.blin@gmail.com wrote:
- {FROM_PROTOCOL_INFO, FROM_PROTOCOL_INFO},
};
static const int ws_socktype_map[][2] = @@ -304,6 +305,7 @@ static const int ws_socktype_map[][2] = MAP_OPTION( SOCK_DGRAM ), MAP_OPTION( SOCK_STREAM ), MAP_OPTION( SOCK_RAW ),
- {FROM_PROTOCOL_INFO, FROM_PROTOCOL_INFO},
};
static const int ws_proto_map[][2] = @@ -314,6 +316,7 @@ static const int ws_proto_map[][2] = MAP_OPTION( IPPROTO_ICMP ), MAP_OPTION( IPPROTO_IGMP ), MAP_OPTION( IPPROTO_RAW ),
- {FROM_PROTOCOL_INFO, FROM_PROTOCOL_INFO},
};
Although it was the case before, it's a mistake to have a comma after the last struct member initializer.
- if (lpProtocolInfo) {
if (af == FROM_PROTOCOL_INFO)af = lpProtocolInfo->iAddressFamily;if (type == FROM_PROTOCOL_INFO)type = lpProtocolInfo->iSocketType;if (protocol == FROM_PROTOCOL_INFO)protocol = lpProtocolInfo->iProtocol;
Please keep opening brace style/indentation consistent with code you have removed.
--- a/include/winsock.h +++ b/include/winsock.h @@ -146,6 +146,7 @@ extern "C" { /* proper 4-byte packing */ #include <pshpack4.h>
+#define FROM_PROTOCOL_INFO (-1)
PSDK has it in winsock2.h
On Sunday 25 May 2008 16:19:24 Dmitry Timoshkov wrote:
@@ -314,6 +316,7 @@ static const int ws_proto_map[][2] = MAP_OPTION( IPPROTO_ICMP ), MAP_OPTION( IPPROTO_IGMP ), MAP_OPTION( IPPROTO_RAW ),
- {FROM_PROTOCOL_INFO, FROM_PROTOCOL_INFO},
};
Although it was the case before, it's a mistake to have a comma after the last struct member initializer.
It is? K&R seems to use both, so I preferred to keep it similar to all the other struct member initializers.
- if (lpProtocolInfo) {
if (af == FROM_PROTOCOL_INFO)af = lpProtocolInfo->iAddressFamily;if (type == FROM_PROTOCOL_INFO)type = lpProtocolInfo->iSocketType;if (protocol == FROM_PROTOCOL_INFO)protocol = lpProtocolInfo->iProtocol;Please keep opening brace style/indentation consistent with code you have removed.
The function is a mess with brace style and indentation. I've used the same indentaion as the if right above this one. The function wildly mixes two-space, three-space and four-space indents. In this case, I used two-space indents for the outer if and four-space indents for the inner ifs for better readability.
--- a/include/winsock.h +++ b/include/winsock.h @@ -146,6 +146,7 @@ extern "C" { /* proper 4-byte packing */ #include <pshpack4.h>
+#define FROM_PROTOCOL_INFO (-1)
PSDK has it in winsock2.h
PSDK has the AF_*, PF_*, SOCK_* and IPPROTO_* defines in winsock2.h as well, Wine hasn't. I preferred to keep them together.
I don't fully understand why people suddenly start getting picky about stuff like this when noone previously gave a damn. As it's not acceptable to just go over this mess with indent, I don't see how conforming to one sort of messy indentation is preferrable to another sort of messy indentation.
Seriously, this patch fixes a bug, take it or leave it and fix it yourself. Kai
"Kai Blin" kai.blin@gmail.com wrote:
- {FROM_PROTOCOL_INFO, FROM_PROTOCOL_INFO},
};
Although it was the case before, it's a mistake to have a comma after the last struct member initializer.
It is? K&R seems to use both, so I preferred to keep it similar to all the other struct member initializers.
I said "a mistake" not "an error". Although the compiler forgives this, what the last comma is serving for?
- if (lpProtocolInfo) {
if (af == FROM_PROTOCOL_INFO)af = lpProtocolInfo->iAddressFamily;if (type == FROM_PROTOCOL_INFO)type = lpProtocolInfo->iSocketType;if (protocol == FROM_PROTOCOL_INFO)protocol = lpProtocolInfo->iProtocol;Please keep opening brace style/indentation consistent with code you have removed.
The function is a mess with brace style and indentation. I've used the same indentaion as the if right above this one. The function wildly mixes two-space, three-space and four-space indents. In this case, I used two-space indents for the outer if and four-space indents for the inner ifs for better readability.
Even if the overall style is a mess it's not a solution to add the code which mixes 2 and 4 spaces indentation, and the code you have removed used a consistent style.
--- a/include/winsock.h +++ b/include/winsock.h @@ -146,6 +146,7 @@ extern "C" { /* proper 4-byte packing */ #include <pshpack4.h>
+#define FROM_PROTOCOL_INFO (-1)
PSDK has it in winsock2.h
PSDK has the AF_*, PF_*, SOCK_* and IPPROTO_* defines in winsock2.h as well, Wine hasn't. I preferred to keep them together.
How much efforts would it take to put 1 line to a proper .h file?
I don't fully understand why people suddenly start getting picky about stuff like this when noone previously gave a damn. As it's not acceptable to just go over this mess with indent, I don't see how conforming to one sort of messy indentation is preferrable to another sort of messy indentation.
Seriously, this patch fixes a bug, take it or leave it and fix it yourself.
All the patches sent to wine-patches fix a bug, not of them are applied due to different reasons. You are not a first timer, and know the rules.
2008/5/26 Dmitry Timoshkov dmitry@codeweavers.com:
"Kai Blin" kai.blin@gmail.com wrote:
- {FROM_PROTOCOL_INFO, FROM_PROTOCOL_INFO},
};
Although it was the case before, it's a mistake to have a comma after the last struct member initializer.
It is? K&R seems to use both, so I preferred to keep it similar to all the other struct member initializers.
I said "a mistake" not "an error". Although the compiler forgives this, what the last comma is serving for?
The C standard explicitly allows the comma at the end of initializer lists, this would be a style issue at best.
"H. Verbeet" hverbeet@gmail.com wrote:
The C standard explicitly allows the comma at the end of initializer lists, this would be a style issue at best.
I mentioned it in hope that it will be fixed in a following patch, since the patch needs to be resent anyway.
On Monday 26 May 2008 08:54:28 Dmitry Timoshkov wrote:
"H. Verbeet" hverbeet@gmail.com wrote:
The C standard explicitly allows the comma at the end of initializer lists, this would be a style issue at best.
I mentioned it in hope that it will be fixed in a following patch, since the patch needs to be resent anyway.
The one thing I can't figure out is when it's appropriate to be consistent to the style already used in the file (which you complained about later in the patch) and when it's appropriate to follow a "correct" style. (I have to add that I never heard of that particular one before, and the Wine developer's guide talks about patch style, not about coding style.)
My issue is not that I need to change things in my patch. My issue is that I don't understand the reason for having to change things. It seems random. While it's trivial to fix this particular patch, I've learned nothing about how to initialize the pseudo-random generator that does the "consistency or correct style" decision for me next time.
Cheers, Kai
H. Verbeet wrote:
The C standard explicitly allows the comma at the end of initializer lists, this would be a style issue at best.
Not to contradict that, but, FWIW, I believe that a comma at the end of an enum list - specifically - is incorrect in ANSI C. The following code, compiled with -pedantic, produces a warning for the enum, alone.
int main() { enum days { Sunday, Monday, Tuesday, }; char s[] = { 'h', 'e', 'l', 'l', 'o', '\0', }; struct st_tag {int i; int j;} st = { 1, 2, };
return 0; }
compiled with "gcc [-ansi] -pedantic -o test test.c" produces "test.c:3: warning: comma at end of enumerator list".
I believe this exception has been removed in C99.
On Monday 26 May 2008 05:08:46 Dmitry Timoshkov wrote:
"Kai Blin" kai.blin@gmail.com wrote:
- {FROM_PROTOCOL_INFO, FROM_PROTOCOL_INFO},
};
Although it was the case before, it's a mistake to have a comma after the last struct member initializer.
It is? K&R seems to use both, so I preferred to keep it similar to all the other struct member initializers.
I said "a mistake" not "an error". Although the compiler forgives this, what the last comma is serving for?
The C grammar printed in my edition of K&R says the following about initializers
initializer: assignment-expression { initializer-list } { initializer-list , }
so it looks like it's in the C standard that both are perfectly valid. I used the perfectly valid form the rest of the file uses.
- if (lpProtocolInfo) {
if (af == FROM_PROTOCOL_INFO)af = lpProtocolInfo->iAddressFamily;if (type == FROM_PROTOCOL_INFO)type = lpProtocolInfo->iSocketType;if (protocol == FROM_PROTOCOL_INFO)protocol = lpProtocolInfo->iProtocol;Please keep opening brace style/indentation consistent with code you have removed.
The function is a mess with brace style and indentation. I've used the same indentaion as the if right above this one. The function wildly mixes two-space, three-space and four-space indents. In this case, I used two-space indents for the outer if and four-space indents for the inner ifs for better readability.
Even if the overall style is a mess it's not a solution to add the code which mixes 2 and 4 spaces indentation, and the code you have removed used a consistent style.
Ok. Fine. How about we finally agree on a coding standard, document that somewhere and require all new patches to conform to that? Then at least one wouldn't have to guess which broken indentation/braces to follow.
PSDK has the AF_*, PF_*, SOCK_* and IPPROTO_* defines in winsock2.h as well, Wine hasn't. I preferred to keep them together.
How much efforts would it take to put 1 line to a proper .h file?
You're not even looking at what I wrote, are you? FROM_PROTOCOL_INFO is used like an address family define, and I tried to keep all of them together.
All the patches sent to wine-patches fix a bug, not of them are applied due to different reasons. You are not a first timer, and know the rules.
What rules are you referring to? The rule that says when adding code to a file that has no obvious dominating code style, I should pick the code style from below the code I add not the code style from above the code I add? I didn't know that one.
I welcome code review that points out errors in my code. Code review that alternates between requiring me to be "formally correct" and "consistent to the code" in a seemingly random fashion is different.
So you don't care about this patch because you don't have a customer who needs it, I don't care about this patch because I can spend my free time on other things. Let's just drop it then.
Kai
"Kai Blin" kai.blin@gmail.com wrote:
The C grammar printed in my edition of K&R says the following about initializers
initializer: assignment-expression { initializer-list } { initializer-list , }
so it looks like it's in the C standard that both are perfectly valid. I used the perfectly valid form the rest of the file uses.
It's valid, and it's useful to avoid explicit structure members initializers, but in the case of array initializers (which is the case) IMO it looks like a typo.
The function is a mess with brace style and indentation. I've used the same indentaion as the if right above this one. The function wildly mixes two-space, three-space and four-space indents. In this case, I used two-space indents for the outer if and four-space indents for the inner ifs for better readability.
Even if the overall style is a mess it's not a solution to add the code which mixes 2 and 4 spaces indentation, and the code you have removed used a consistent style.
Ok. Fine. How about we finally agree on a coding standard, document that somewhere and require all new patches to conform to that? Then at least one wouldn't have to guess which broken indentation/braces to follow.
It's silly IMO to demand a coding standard when somebody pointed out to an obviously sloppy code style on your side.
PSDK has the AF_*, PF_*, SOCK_* and IPPROTO_* defines in winsock2.h as well, Wine hasn't. I preferred to keep them together.
How much efforts would it take to put 1 line to a proper .h file?
You're not even looking at what I wrote, are you? FROM_PROTOCOL_INFO is used like an address family define, and I tried to keep all of them together.
All I say is that it's not OK to put the code at wrong place, even if there is existing code which is at the wrong place already.
[skipped]
So you don't care about this patch because you don't have a customer who needs it,
I haven't said anything about customer needs, there is no need to speculate about it.
I don't care about this patch because I can spend my free time on other things. Let's just drop it then.
You already spent more time arguing than it would take to fix the patch, and resend.
On Sun, 25 May 2008, Kai Blin wrote: [...]
--- a/include/winsock.h +++ b/include/winsock.h @@ -146,6 +146,7 @@ extern "C" { /* proper 4-byte packing */ #include <pshpack4.h>
+#define FROM_PROTOCOL_INFO (-1)
PSDK has it in winsock2.h
PSDK has the AF_*, PF_*, SOCK_* and IPPROTO_* defines in winsock2.h as well, Wine hasn't.
That's because MS likes to duplicate things a lot. So in the PSDK AF_*, PF_*, SOCK_* and IPPROTO_* are defined in _both_ winsock.h and winsock2.h. In Wine we're defining them in winsock.h only, but we're then #including winsock.h in winsock2.h, using __WINE_WINSOCK2__ to solve the handful of incompatibilities.
But the PSDK defines FROM_PROTOCOL_INFO _only_ in winsock2.h so it's important to do the same in Wine.
I preferred to keep them together.
It's important that the Wine headers match the PSDK ones as best as possible. Differences usually result in Windows code that does not compile with Wine, bad for Winelib, or in the Wine conformance tests that don't compile with the PSDK.
Anyway, I'm sending a patch to add FROM_PROTOCOL_INFO to winsock2.h...
On Sun, 25 May 2008, Dmitry Timoshkov wrote:
"Kai Blin" kai.blin@gmail.com wrote: [...]
MAP_OPTION( IPPROTO_IGMP ), MAP_OPTION( IPPROTO_RAW ),
- {FROM_PROTOCOL_INFO, FROM_PROTOCOL_INFO},
};
Although it was the case before, it's a mistake to have a comma after the last struct member initializer. [...]
Placing a comma should be OK if there are chances the struct or enum will ever grow. This prevents later patching the line to add the comma, possibly creating confusion about who is the author of that line.
On the other hand, no comma at the end of an enum or struct memeber that we want to always be the last one (COUNT_OF_WHATEVER_OPTIONS, custom_data_area[], etc.) helps avoiding trivial errors as it forces one to add that comma to compile the code.
Paul Chitescu
-
Andrew Talbot -
Dmitry Timoshkov -
Francois Gouget -
H. Verbeet -
Kai Blin -
Paul Chitescu