[tor-bugs] #3726 [Pluggable transport]: obfsproxy needs contrib/checkSpace.pl

[Tor Bug Tracker & Wiki]

#3726: obfsproxy needs contrib/checkSpace.pl
---------------------------------+------------------------------------------
 Reporter:  asn                  |          Owner:  asn         
     Type:  defect               |         Status:  needs_review
 Priority:  normal               |      Milestone:              
Component:  Pluggable transport  |        Version:              
 Keywords:                       |         Parent:  #3591       
   Points:                       |   Actualpoints:              
---------------------------------+------------------------------------------

Comment(by asn):

 I just finished a 6 hours session of looking at uncrustify...

 You can see my custom config and the results of uncrustify, in branch
 `bug3726_uncrustify_test` in
 `https://git.gitorious.org/obfsproxy/obfsproxy.git`:
 https://gitorious.org/obfsproxy/obfsproxy/commits/bug3726_uncrustify_test
 https://gitorious.org/obfsproxy/obfsproxy/commit/dae09f28999d5ce29dc6a4137ad1e2379e8cc5e8

 All in all, it seems useful and very versatile, but it still has its
 issues:

 == Issues: ==
 - Sometimes it splits function arguments very stupidly:
 {{{
  struct evutil_addrinfo *resolve_address_port(const char *address,
 -                                             int nodns, int passive,
 +                                             int nodns,
 +                                             int passive,
                                               const char *default_port);
 }}}
 or much worse:
 {{{
  void
 -smartlist_uniq(smartlist_t *sl,
 -               int (*compare)(const void **a, const void **b),
 -               void (*free_fn)(void *a))
 +smartlist_uniq(smartlist_t *sl,int (*compare)(const void **a,
 +                                              const void **b),void
 (*free_fn)(
 +                 void *a))
 }}}

 ----

 - There is a boolean option called `indent_func_call_param` for which:
 {{{
 True:  indent continued function call parameters one indent level
 False: align parameters under the open paren
 }}}

 So if it's true, this ugly thing happens:
 {{{
        memmove(sl->list + idx + 1, sl->list + idx,
  -              sizeof(void*)*(sl->num_used-idx));
  +        sizeof(void*)*(sl->num_used-idx));
 }}}

 but if it's false, this other ugly thing happens:

 {{{
     SMARTLIST_FOREACH(sl1, const char *, cp1, {
  -      const char *cp2 = smartlist_get(sl2, cp1_sl_idx);
  -      if (strcmp(cp1, cp2))
  -        return 0;
  -    });
  +                      const char *cp2 = smartlist_get(sl2, cp1_sl_idx);
  +                      if (strcmp(cp1, cp2))
  +                        return 0;
  +                    });
 }}}

 which is not the way we usually align such macros.
 I left it on 'false' in my configuration.

 ----

 - It will not think twice before beautifying macro definitions, but it
 won't align the '\' newline escapes afterwards. I haven't managed to find
 a configuration option for this (`indent_align_string` doesn't seem to do
 it).

 {{{
  #define SMARTLIST_FOREACH_END(var)              \
 -    var = NULL;                                 \
 +  var = NULL;                                 \
    } } while (0)
 }}}

 == Missing features ==
 I'll mention here some features I would have enjoyed, in case an
 uncrustify guru comes in and points me to the correct configuration
 option.

 - A way to specify whether we like the final "*/" of a comment in a new
 line or not:
 {{{
 /** Advance the iterator <b>iter</b> a single step to the next entry,
 removing
  * the current entry, and return its new value.
  */
 }}}
 versus
 {{{
 /** Set *<b>keyp</b> and *<b>valp</b> to the current entry pointed to by
  * iter. */
 }}}

 ----

 - A way to tell it whether we like aligned struct members or not, so that
 it can break them down if we don't:
 {{{
 struct conn_t {
   config_t           *cfg;
   char               *peername;
   circuit_t          *circuit;
   struct bufferevent *buffer;
   enum listen_mode mode;
 };
 }}}


 == Other comments ==
 Most changes in my branch were caused by these two options:
 {{{
 # Whether to put a star on subsequent comment lines
 cmt_star_cont                            = true    # false/true
 }}}
 and
 {{{
 # The number of blank lines after a block of variable definitions at the
 top of a function body.
 # 0=no change (default)
 nl_func_var_def_blk                      = 1        # number
 }}}

 I'm not sure if I like the former option (although tor seems to like it),
 and I think I like the latter. By disabling these two options, we should
 get a more conservative diff.

 Also, if we end up using uncrustify, I'm not sure if we should use it on
 sha256.c (tor's checkSpace.pl doesn't check sha256.c either), or
 container.[ch] (maybe we should keep container.[ch] as similar to its tor
 version as possible).

 == Thoughts ==

 I think I'll stop playing with uncrustify for today.

 I'll do some more tests this week, and see whether I'm wrong with any of
 my complaints above (very likely).
 I'll then make a new branch, with the new config and the new changes (and
 a better Makefile), and put it here on display. If we like it, we can add
 an uncrustify configuration file to obfsproxy. If we don't like it, we can
 use checkSpace.pl while waiting for uncrustify to become better (or till
 we hack the features we want in, ourselves...).

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/3726#comment:4>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online