[Pidgin] #3207: Request nickname for authorization request
Pidgin
trac at pidgin.im
Sat Jun 6 11:11:53 EDT 2009
#3207: Request nickname for authorization request
------------------------------------+---------------------------------------
Reporter: collin | Owner:
Type: patch | Status: new
Milestone: Patches Needing Review | Component: pidgin (gtk)
Version: 2.0.2 | Resolution:
Keywords: |
------------------------------------+---------------------------------------
Comment(by resiak):
Some style points:
* We don't generally add copyright headers to individual files. I see that
in some cases there are some copyright headers in two of the changed
files, though. In any case, ComBOTS should be added to COPYRIGHT.
* Code shouldn't be commented out, it should be removed outright. The
rationale for removing it lives in the commit message.
* The brace style is wrong. The rest of `oscar.c` cuddles the opening
brace onto the same line as the `if`.
* I really hate bitfielding integers, particularly when it won't make any
difference (as I doubt it will for `auth_request`).
Functional points:
{{{
data->name = g_strdup_printf("%u", info->uin);
}}}
Is this ever freed?
{{{
if (info->auth_request)
{
g_free(info->reason);
}
}}}
Does this mean that `reason` might or might not be a freshly-allocated
string, depending on `auth_request`? I'd rather it was consistently dup'd
and freed. If instead it's because `reason` isn't set unless
`auth_request`, `g_free(NULL);` is a perfectly safe no-op so the `if` can
be removed.
The patch looks like a good idea to me. I appreciate that the patch
submitter may have lost interest by now, though. Collin, would you like to
update the patch? If not, I guess one of us should do so.
--
Ticket URL: <http://developer.pidgin.im/ticket/3207#comment:7>
Pidgin <http://pidgin.im>
Pidgin
More information about the Tracker
mailing list