#20048 closed enhancement (implemented)

Introduce `smartlist_add_strdup()` function

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: easy, TorCoreTeam201610
Cc: Actual Points:
Parent ID: Points: 0.3
Reviewer: Sponsor:

Description

There are many places in the code (and the tests) where we do the following pattern:
smartlist_add(sl, tor_strdup(str))

Some examples:

routerparse.c:                    smartlist_add(ns->package_lines, tor_strdup(t->args[0])));
routerparse.c:    smartlist_add(ns->known_flags, tor_strdup(tok->args[i]));
routerparse.c:      smartlist_add(ns->net_params, tor_strdup(tok->args[i]));
routerparse.c:      smartlist_add(ns->weight_params, tor_strdup(tok->args[i]));
routerparse.c:        smartlist_add(md->family, tor_strdup(tok->args[i]));
routerset.c:    smartlist_add(set->country_names, tor_strdup("??"));
routerset.c:    smartlist_add(set->list, tor_strdup("{??}"));
routerset.c:    smartlist_add(set->country_names, tor_strdup("a1"));
routerset.c:    smartlist_add(set->list, tor_strdup("{a1}"));

One could imagine a smartlist_add_strdup() function that does this for the developer, and can be used in places where this pattern is used repeatedly.

It might simplify logic in a few places, or maybe it will confuse peole who grep for tor_strdup searching for allocations.

If people think this is worth doing, let's do it!

Child Tickets

Attachments (4)

big.diff (40.4 KB) - added by icanhasaccount 13 months ago.
implement smartlist_add_strdup
bug20048_implement_smartlist_add_strdup.diff (1.4 KB) - added by icanhasaccount 13 months ago.
Implement smartlist_add_strdup
bug20048_use_smartlist_add_strdup.diff (24.3 KB) - added by icanhasaccount 13 months ago.
Use smartlist_add_strdup for src
bug20048_use_smartlist_add_strdup_test.diff (15.1 KB) - added by icanhasaccount 13 months ago.
Use smartlist_add_strdup in src/test/

Download all attachments as: .zip

Change History (16)

comment:1 Changed 15 months ago by nickm

yes; please do; would merge.

comment:2 Changed 15 months ago by nickm

An (untested) coccinelle script to transform the code here might look something like this. (If it works.)

@@
expression a;
expression b;
@@
- smartlist_add
+ smartlist_add_strdup
   (a,
- tor_strdup(
   b
- )
  )     
@@

Changed 13 months ago by icanhasaccount

Attachment: big.diff added

implement smartlist_add_strdup

comment:3 Changed 13 months ago by icanhasaccount

I attempted this - see attached. (coccinelle is handy - never used it before!) There are quite a few tests that could be shortened as well but I thought I'd wait for feedback before doing that.

comment:4 Changed 13 months ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.0.x-final
Status: newneeds_revision
Type: taskenhancement

Looks good to me, but needs a change file.
See https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md#n56

Typically, we do 2 commits for an automated change like this: one to set up the new function, and one big one where the automated script is applied (and included in the commit message). That way, anyone can verify the big commit by running the coccinelle script again.

I'm not sure why we'd want to shorten the tests.

comment:5 Changed 13 months ago by icanhasaccount

Thank you for checking and for the feedback - I'll break the diff up into parts with a change file.

Re the shortening thing - I explained that really badly. When reviewing the diff I noticed there are a few places (mostly in the test directory) where the pattern is:

char *foo = tor_strdup("bar");
....
smartlist_add(sl, foo);

By shortening I meant I could go through and change those to use smartlist_add_strdup where appropriate afterwards.

Thanks again for your patience/help!

comment:6 in reply to:  5 Changed 13 months ago by teor

Replying to icanhasaccount:

Thank you for checking and for the feedback - I'll break the diff up into parts with a change file.

Thanks!

Re the shortening thing - I explained that really badly. When reviewing the diff I noticed there are a few places (mostly in the test directory) where the pattern is:

char *foo = tor_strdup("bar");
....
smartlist_add(sl, foo);

By shortening I meant I could go through and change those to use smartlist_add_strdup where appropriate afterwards.

Yes, you can add another commit for that.
(It's probably not worth making the coccinelle script smarter.)
Are there any instances in the non-test code?

comment:7 Changed 13 months ago by icanhasaccount

Nope, I don't believe so (I wasn't 100% certian so I went back and checked each smartlist_add(xx, yy) in files where tor_strdup() is used (outside of test/) and didn't spot any).

Changed 13 months ago by icanhasaccount

Implement smartlist_add_strdup

Changed 13 months ago by icanhasaccount

Use smartlist_add_strdup for src

Changed 13 months ago by icanhasaccount

Use smartlist_add_strdup in src/test/

comment:8 Changed 13 months ago by icanhasaccount

Heya - I split the patch into 3 parts - one with the implementation, one to use smartlist_add_strdup for everywhere but src/test and a final patch for src/test.

I had to change the following files in src/test by hand:

test_shared_random.c
test_routerset.c
test_containers.c
test_dir.c

due to coccinelle errors - it really doesn't seem like the NS macro in particular. I tried various incantations/options without success but if anyone has any input on how to make it work properly I'll gladly redo. Also is posting the patches here okay or should I use github or somesuch thing?

comment:9 in reply to:  8 Changed 13 months ago by cypherpunks

Replying to icanhasaccount:

Also is posting the patches here okay or should I use github or somesuch thing?

See https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md#n12

comment:10 Changed 13 months ago by icanhasaccount

Thank you all for your patience/help. I've read through the coding standards doc properly and setup chutney etc. I've put a branch with the changes thus far on github: https://github.com/mintytoast/tor-work/tree/bug20048

comment:11 Changed 13 months ago by teor

Keywords: TorCoreTeam201610 added
Status: needs_revisionmerge_ready

Looks good to me, let's get this merged.
And thanks!

comment:12 Changed 13 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged; thank you!

Note: See TracTickets for help on using tickets.