#13827: Cell handling code duplication in channel.c
 Reporter:  rl1987                             |          Owner:  pingl
     Type:  defect                             |         Status:
                                               |  needs_revision
 Priority:  Medium                             |      Milestone:  Tor:
                                               |  0.2.9.x-final
Component:  Core Tor/Tor                       |        Version:
 Severity:  Normal                             |     Resolution:
 Keywords:  refactoring, easy, review-group-9  |  Actual Points:
Parent ID:                                     |         Points:  0.1
 Reviewer:  dgoulet                            |        Sponsor:
Changes (by dgoulet):

 * status:  needs_review => needs_revision
 * reviewer:   => dgoulet
 * points:  small/medium => 0.1


 (I've pull your patch into `bug13827_029_01` if anyone wants to see it
 from git.tpo in my repo.)

 DG1: This worries me:
 -  q.u.var.var_cell = var_cell;
 +  q.u.fixed.cell = cell;

 In theory, that could work since it's a union and all cell points there
 but kind of recipe for disaster and bad semantic. What you could do is
 take a reference on the right cell member of the union while in the switch
 case and then assign it after.

 DG2: Can't you use `CELL_QUEUE_*` as the cell type?

 DG3: Few things. I would rename `ctype` to `cell_type`. The switch case
 MUST have a default branch that you could do a BUG() on and bail. Finally,
 no need for extra space between the case and the end of the function.

 Thanks for this!

Ticket URL: <https://trac.torproject.org/projects/tor/ticket/13827#comment:12>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
tor-bugs mailing list

Reply via email to