From 7faf115dfffaf12cdae98eac71fc6811059c6657 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 17 Jul 2012 09:33:38 -0400 Subject: Change all SMARTLIST_FOREACH loops of >=10 lines to use BEGIN/END The SMARTLIST_FOREACH macro is more convenient than BEGIN/END when you have a nice short loop body, but using it for long bodies makes your preprocessor tell the compiler that all the code is on the same line. That causes grief, since compiler warnings and debugger lines will all refer to that one line. So, here's a new style rule: SMARTLIST_FOREACH blocks need to be short. --- changes/smartlist_foreach | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changes/smartlist_foreach (limited to 'changes') diff --git a/changes/smartlist_foreach b/changes/smartlist_foreach new file mode 100644 index 0000000000..d1e3505468 --- /dev/null +++ b/changes/smartlist_foreach @@ -0,0 +1,4 @@ + o Code simplification and refactoring: + - Do not allow the body of any SMARTLIST_FOREACH block to exceed + 10 lines. Doing so in the past has led to hard-to-debug code. + The new style is to use the SMARTLIST_FOREACH_{BEGIN,END} pair. -- cgit v1.2.3-54-g00ecf From efdf6c711810cfa951741071cb7b67c3a76f2651 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 17 Jul 2012 10:41:24 -0400 Subject: Fix the remaining instances of nexted SMARTLIST_FOREACH --- changes/smartlist_foreach | 2 ++ src/or/networkstatus.c | 8 ++++---- src/or/rendservice.c | 8 ++++---- 3 files changed, 10 insertions(+), 8 deletions(-) (limited to 'changes') diff --git a/changes/smartlist_foreach b/changes/smartlist_foreach index d1e3505468..0a6c960427 100644 --- a/changes/smartlist_foreach +++ b/changes/smartlist_foreach @@ -2,3 +2,5 @@ - Do not allow the body of any SMARTLIST_FOREACH block to exceed 10 lines. Doing so in the past has led to hard-to-debug code. The new style is to use the SMARTLIST_FOREACH_{BEGIN,END} pair. + - Do not allow SMARTLIST_FOREACH blocks to nest. Any nested block + ought to be using SMARTLIST_FOREACH_{BEGIN,END}. diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 1979250c5b..fadaf90da4 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -128,12 +128,12 @@ networkstatus_reset_download_failures(void) { int i; const smartlist_t *networkstatus_v2_list = networkstatus_get_v2_list(); - SMARTLIST_FOREACH(networkstatus_v2_list, networkstatus_v2_t *, ns, - SMARTLIST_FOREACH(ns->entries, routerstatus_t *, rs, - { + SMARTLIST_FOREACH_BEGIN(networkstatus_v2_list, networkstatus_v2_t *, ns) { + SMARTLIST_FOREACH_BEGIN(ns->entries, routerstatus_t *, rs) { if (!router_get_by_descriptor_digest(rs->descriptor_digest)) rs->need_to_mirror = 1; - }));; + } SMARTLIST_FOREACH_END(rs); + } SMARTLIST_FOREACH_END(ns); for (i=0; i < N_CONSENSUS_FLAVORS; ++i) download_status_reset(&consensus_dl_status[i]); diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 6a51874699..6af4778dfc 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -500,16 +500,16 @@ rend_config_services(const or_options_t *options, int validate_only) /* Copy introduction points to new services. */ /* XXXX This is O(n^2), but it's only called on reconfigure, so it's * probably ok? */ - SMARTLIST_FOREACH(rend_service_list, rend_service_t *, new, { - SMARTLIST_FOREACH(old_service_list, rend_service_t *, old, { + SMARTLIST_FOREACH_BEGIN(rend_service_list, rend_service_t *, new) { + SMARTLIST_FOREACH_BEGIN(old_service_list, rend_service_t *, old) { if (!strcmp(old->directory, new->directory)) { smartlist_add_all(new->intro_nodes, old->intro_nodes); smartlist_clear(old->intro_nodes); smartlist_add(surviving_services, old); break; } - }); - }); + } SMARTLIST_FOREACH_END(old); + } SMARTLIST_FOREACH_END(new); /* Close introduction circuits of services we don't serve anymore. */ /* XXXX it would be nicer if we had a nicer abstraction to use here, -- cgit v1.2.3-54-g00ecf From 78dec943074c26747abc7a68cd6aec5269100569 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 18 Jul 2012 10:12:19 -0400 Subject: Tweaks to 6400 changes file and docs as suggested by arma --- changes/smartlist_foreach | 10 ++++++---- src/common/container.h | 24 +++++++++++++++--------- 2 files changed, 21 insertions(+), 13 deletions(-) (limited to 'changes') diff --git a/changes/smartlist_foreach b/changes/smartlist_foreach index 0a6c960427..2fd3a1a85c 100644 --- a/changes/smartlist_foreach +++ b/changes/smartlist_foreach @@ -1,6 +1,8 @@ o Code simplification and refactoring: - - Do not allow the body of any SMARTLIST_FOREACH block to exceed - 10 lines. Doing so in the past has led to hard-to-debug code. + - Do not use SMARTLIST_FOREACH for any loop whose body exceeds + 10 lines. Doing so in the past has led to hard-to-debug code. The new style is to use the SMARTLIST_FOREACH_{BEGIN,END} pair. - - Do not allow SMARTLIST_FOREACH blocks to nest. Any nested block - ought to be using SMARTLIST_FOREACH_{BEGIN,END}. + Issue 6400. + - Do not nest SMARTLIST_FOREACH blocks within one another. Any + nested block ought to be using SMARTLIST_FOREACH_{BEGIN,END}. + Issue 6400. diff --git a/src/common/container.h b/src/common/container.h index c11c2eb82f..dab3b83f37 100644 --- a/src/common/container.h +++ b/src/common/container.h @@ -142,9 +142,9 @@ char *smartlist_join_strings2(smartlist_t *sl, const char *join, /** Iterate over the items in a smartlist sl, in order. For each item, * assign it to a new local variable of type type named var, and - * execute the statement cmd. Inside the loop, the loop index can - * be accessed as var_sl_idx and the length of the list can be accessed - * as var_sl_len. + * execute the statements inside the loop body. Inside the loop, the loop + * index can be accessed as var_sl_idx and the length of the list can + * be accessed as var_sl_len. * * NOTE: Do not change the length of the list while the loop is in progress, * unless you adjust the _sl_len variable correspondingly. See second example @@ -153,23 +153,21 @@ char *smartlist_join_strings2(smartlist_t *sl, const char *join, * Example use: * 
  *   smartlist_t *list = smartlist_split("A:B:C", ":", 0, 0);
- *   SMARTLIST_FOREACH(list, char *, cp,
- *   {
+ *   SMARTLIST_FOREACH_BEGIN(list, char *, cp) {
  *     printf("%d: %s\n", cp_sl_idx, cp);
  *     tor_free(cp);
- *   });
+ *   } SMARTLIST_FOREACH_END(cp);
  *   smartlist_free(list);
  *
* * Example use (advanced): * 
- *   SMARTLIST_FOREACH(list, char *, cp,
- *   {
+ *   SMARTLIST_FOREACH_BEGIN(list, char *, cp) {
  *     if (!strcmp(cp, "junk")) {
  *       tor_free(cp);
  *       SMARTLIST_DEL_CURRENT(list, cp);
  *     }
- *   });
+ *   } SMARTLIST_FOREACH_END(cp);
  *
*/ /* Note: these macros use token pasting, and reach into smartlist internals. @@ -218,6 +216,14 @@ char *smartlist_join_strings2(smartlist_t *sl, const char *join, var = NULL; \ } STMT_END +/** + * An alias for SMARTLIST_FOREACH_BEGIN and SMARTLIST_FOREACH_END, using + * cmd as the loop body. This wrapper is here for convenience with + * very short loops. + * + * By convention, we do not use this for loops which nest, or for loops over + * 10 lines or so. Use SMARTLIST_FOREACH_{BEGIN,END} for those. + */ #define SMARTLIST_FOREACH(sl, type, var, cmd) \ SMARTLIST_FOREACH_BEGIN(sl,type,var) { \ cmd; \ -- cgit v1.2.3-54-g00ecf