[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #13827 [Core Tor/Tor]: Cell handling code duplication in channel.c
#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
Comment:
(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
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs