Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#6400 closed enhancement (implemented)

Eliminate long SMARTLIST_FOREACH statements

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I introduced SMARTLIST_FOREACH_{BEGIN,END} a while back so that we wouldn't have the attendant problems from having large SMARTLIST_FOREACH() blocks. But there are still some of those lingering in the codebase.

I propose a 10-line maximum for a SMARTLIST_FOREACH statement, and that we prohibit them from nesting.

I was going to do this on 0.2.3, but I realize that if somebody else proposed doing this on 0.2.3 I would say "no", so this is 0.2.4 material.

Child Tickets

Attachments (1)

big-loops2.py (967 bytes) - added by nickm 7 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 7 years ago by nickm

Status: newneeds_review

see branch "smartlist_shorten" in my public repo. It's based on 0.2.3, but should merge cleanly into 0.2.4.

Changed 7 years ago by nickm

Attachment: big-loops2.py added

comment:2 Changed 7 years ago by nickm

I've attached the script I used to find large loops. It's a monstrous kludge based on how the preprocessor behaves. To use it, temporarily add "(void)100;" to the SMARTLIST_FOREACH implementation at the start of the loop body, then invoke the script as "python big-loops2.py src/*/*.c src/*/*/*.c". It relies on the preprocessor acting a little like gcc in how it squashes lines.

comment:3 Changed 7 years ago by arma

s/nexted/nested/

In the changes file when you say "Do not allow", does anything enforce it in an ongoing way? If no, it's misleading.

Would be great if the changes file referenced this bug number.

The SMARTLIST_FOREACH() definition should have a comment telling you when it's inappropriate to use it.

The code changes look good.

I wouldn't oppose putting this patch into maint-0.2.3, on the "if we later want to backport a bugfix on any of the changed lines, it will make the backport cleaner" theory.

comment:4 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.3.x-final
Resolution: implemented
Status: needs_reviewclosed

Made the suggested changes. Merging into 0.2.3.x. The tipping point for me is that it's not only a code refactoring; it also helps us get better stack traces and debug output, etc.

comment:5 Changed 7 years ago by nickm

looks like I left "nexted" by mistake. ah well; that should get people to look more closely to see what else I might have messed up. :)

comment:6 Changed 7 years ago by nickm

Keywords: tor-client added

comment:7 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.