Folks,
The students in my software-engineering class are writing IRC clients in Java, and I'm running ngIRCd as a sandbox for them to play in. We noticed ngIRCd doesn't obey the "JOIN 0" command specified in RFC 2812:
JOIN 0 ; Leave all currently joined channels.
http://tools.ietf.org/html/rfc2812#section-3.2.1
I believe the following patch addresses this. Cheers!
Dana Dahlstrom dana+70@cs.ucsd.edu wrote:
The students in my software-engineering class are writing IRC clients in Java, and I'm running ngIRCd as a sandbox for them to play in. We noticed ngIRCd doesn't obey the "JOIN 0" command specified in RFC 2812:
JOIN 0 ; Leave all currently joined channels.
http://tools.ietf.org/html/rfc2812#section-3.2.1
I believe the following patch addresses this. Cheers!
Applied -- thanks for this work. Also thanks for adding test cases -- this is really much appreciated.
On 2008-02-05 at 14:40 +0100, Florian Westphal wrote:
Applied -- thanks for this work. Also thanks for adding test cases -- this is really much appreciated.
It was my pleasure! I wrote the tests first, actually---in conformance with the XP methods we're trying to teach in the class. :)
I like refactoring out part_from_all_channels as a function, but I wonder if it doesn't belong in channel.c instead of irc-channel.c. It looks to me like channel.c contains a bunch of utilities while every function in irc-channel.c handles an actual IRC message and accepts a CLIENT* and a REQUEST* as parameters. Is this convention the project has decided to abandon?
Wherever the code lives, I noticed there's a way to make the loop slightly more elegant:
diff -u -p -r1.42 irc-channel.c --- src/ngircd/irc-channel.c 5 Feb 2008 13:31:50 -0000 1.42 +++ src/ngircd/irc-channel.c 5 Feb 2008 16:15:24 -0000 @@ -51,16 +51,13 @@ static char UNUSED id[] = "$Id: irc-chan static bool part_from_all_channels(CLIENT* client, CLIENT *target) { - CL2CHAN *cl2chan = Channel_FirstChannelOf(target); + CL2CHAN *cl2chan; CHANNEL *chan;
- while (cl2chan) { + while (cl2chan = Channel_FirstChannelOf(target)) { chan = Channel_GetChannel(cl2chan); assert( chan != NULL ); Channel_Part(target, client, Channel_Name(chan), Client_ID(target)); - - /* next */ - cl2chan = Channel_FirstChannelOf(target); } return CONNECTED; }
Dana Dahlstrom dana+70@cs.ucsd.edu wrote:
I like refactoring out part_from_all_channels as a function, but I wonder if it doesn't belong in channel.c instead of irc-channel.c. It looks to me like channel.c contains a bunch of utilities while every function in irc-channel.c handles an actual IRC message and accepts a CLIENT* and a REQUEST* as parameters. Is this convention the project has decided to abandon?
No. But in this case i've decided not do do that. There are no other users of this functionality and moving it to channel.c requires to export a new function. Having it in irc-channel.c means we can keep it static, and gcc then even removes the function call.
Wherever the code lives, I noticed there's a way to make the loop slightly more elegant:
I usually try to avoid assignments inside conditionals; and here is why:
irc-channel.c: In function 'part_from_all_channels': irc-channel.c:57: warning: suggest parentheses around assignment used as truth value
Easily silenced, but this turns
- while (cl2chan) {
- while (cl2chan = Channel_FirstChannelOf(target)) {
into while ((cl2chan = Channel_FirstChannelOf(target))) {
I'm not entirely sure which is more elegant 8-)
Since you felt strong enough about it to actually send a patch i'll apply this (unless vetoed by Alex).
(the generated code is the same, so this really is only about CodingStyle)
Regards, Florian.
Am 05.02.2008 um 18:19 schrieb Florian Westphal:
Dana Dahlstrom dana+70@cs.ucsd.edu wrote:
I like refactoring out part_from_all_channels as a function, but I wonder if it doesn't belong in channel.c instead of irc-channel.c. It looks to me like channel.c contains a bunch of utilities while every function in irc-channel.c handles an actual IRC message and accepts a CLIENT* and a REQUEST* as parameters. Is this convention the project has decided to abandon?
No. But in this case i've decided not do do that. There are no other users of this functionality and moving it to channel.c requires to export a new function. Having it in irc-channel.c means we can keep it static, and gcc then even removes the function call.
Semms to be the correct way: local ("static") helper functions do belong in the module from which they are called.
Wherever the code lives, I noticed there's a way to make the loop slightly more elegant:
I usually try to avoid assignments inside conditionals; and here is why:
irc-channel.c: In function 'part_from_all_channels': irc-channel.c:57: warning: suggest parentheses around assignment used as truth value
Easily silenced, but this turns
- while (cl2chan) {
- while (cl2chan = Channel_FirstChannelOf(target)) {
into while ((cl2chan = Channel_FirstChannelOf(target))) {
I'm not entirely sure which is more elegant 8-)
Since you felt strong enough about it to actually send a patch i'll apply this (unless vetoed by Alex).
(the generated code is the same, so this really is only about CodingStyle)
I'm not sure about this one: in the first place, the variant suggested by Dana looks cleaner, but the "double braces" required to calm down gcc make it ugly. So its ... a little bit ugly either way :-)
So probably the code is more readable the way it is at the moment?
Regards Alex
On 2008-02-05 at 18:55 +0100, Alexander Barton wrote:
I'm not sure about this one: in the first place, the variant suggested by Dana looks cleaner, but the "double braces" required to calm down gcc make it ugly. So its ... a little bit ugly either way :-)
I wasn't going to follow up on this because, well, it seems like a small issue, but since there are 2 posts about it already... :)
My sense is it's generally not a good idea to duplicate even one line of code because it makes maintenance more complex (if only a little). For example, if the call to Channel_FirstChannelOf needed to change somehow, it would have to be changed in 2 places here instead of 1, or else a bug could potentially result.
But, as I wrote to Florian off-list, I don't actually feel all that strongly about it. I'm sure it will be fine either way. :)
Dana
-
Alexander Barton -
Dana Dahlstrom -
Florian Westphal