[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #3726 [Pluggable transport]: obfsproxy needs contrib/checkSpace.pl
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs