[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