[apparmor] [PATCH 05/10] Lindent + hand cleanups compressed-dfa

[Steve Beattie]

On Thu, Mar 10, 2011 at 12:35:55PM -0800, John Johansen wrote:
> Signed-off-by: John Johansen <john.johansen at canonical.com>

If you wish to consider this an ACK you can.

However,

> diff --git a/parser/libapparmor_re/compressed_hfa.cc b/parser/libapparmor_re/compressed_hfa.cc
> index 795d638..24b9e00 100644
> --- a/parser/libapparmor_re/compressed_hfa.cc
> +++ b/parser/libapparmor_re/compressed_hfa.cc
> @@ -70,19 +69,23 @@ TransitionTable::TransitionTable(DFA& dfa, map<uchar, uchar>& eq,
>  	 * transition count.
>  	 */
>  	size_t optimal = 2;
> -	multimap <size_t, State *> order;
> -	vector <pair<size_t, size_t> > free_list;
> +	multimap <size_t, State *>order;
> +	vector <pair <size_t, size_t> >free_list;
>  
> -	for (Partition::iterator i = dfa.states.begin(); i != dfa.states.end(); i++) {
> +	for (Partition::iterator i = dfa.states.begin(); i != dfa.states.end();
> +	     i++) {

^^^ the above is where I kinda would rather see things left on a single line

>  		if (*i == dfa.start || *i == dfa.nonmatching)
>  			continue;
>  		optimal += (*i)->cases.cases.size();
>  		if (flags & DFA_CONTROL_TRANS_HIGH) {
>  			size_t range = 0;
>  			if ((*i)->cases.cases.size())
> -				range = (*i)->cases.cases.rbegin()->first - (*i)->cases.begin()->first;
> -			size_t ord = ((256 - (*i)->cases.cases.size()) << 8) |
> -				(256 - range);
> +				range =
> +				    (*i)->cases.cases.rbegin()->first -
> +				    (*i)->cases.begin()->first;
> +			size_t ord =
> +			    ((256 - (*i)->cases.cases.size()) << 8) | (256 -
> +								       range);

I'd probably push "range);" back onto the preceeding line. I won't point
out other situations, it's up to you if you want to adjust or not.

>  			/* reverse sort by entry count, most entries first */
>  			order.insert(make_pair(ord, *i));
>  		}
> @@ -189,7 +204,7 @@ void TransitionTable::insert_state(vector <pair<size_t, size_t> > &free_list,
>  	if (cases.cases.empty())
>  		goto do_insert;
>  
> -repeat:
> +      repeat:

goto labels, however, are one of the things that indent/Lindent handle
poorly. I really prefer to see them starting in column 0, and, unless
things have changed, that's also the preference (despite the behavior
of Lindent) of LKML.

>  	resize = 0;
>  	/* get the first free entry that won't underflow */
>  	while (x && (x < c)) {
> @@ -225,66 +241,70 @@ repeat:
>  
>  	base = x - c;
>  	for (Cases::iterator j = cases.begin(); j != cases.end(); j++) {
> -	    next_check[base + j->first] = make_pair(j->second, from);
> -	    size_t prev = free_list[base + j->first].first;
> -	    size_t next = free_list[base + j->first].second;
> -	    if (prev)
> -		    free_list[prev].second = next;
> -	    if (next)
> -		    free_list[next].first = prev;
> -	    if (base + j->first == first_free)
> -		    first_free = next;
> +		next_check[base + j->first] = make_pair(j->second, from);
> +		size_t prev = free_list[base + j->first].first;
> +		size_t next = free_list[base + j->first].second;
> +		if (prev)
> +			free_list[prev].second = next;
> +		if (next)
> +			free_list[next].first = prev;
> +		if (base + j->first == first_free)
> +			first_free = next;
>  	}
>  
> -do_insert:
> +      do_insert:

same here.

> --- a/parser/libapparmor_re/compressed_hfa.h
> +++ b/parser/libapparmor_re/compressed_hfa.h
> @@ -29,28 +29,28 @@
>  using namespace std;
>  
>  class TransitionTable {
> -    typedef vector<pair<const State *, size_t> > DefaultBase;
> -    typedef vector<pair<const State *, const State *> > NextCheck;
> -public:
> -    TransitionTable(DFA& dfa, map<uchar, uchar>& eq, dfaflags_t flags);
> -    void dump(ostream& os);
> -    void flex_table(ostream& os, const char *name);
> -    void init_free_list(vector <pair<size_t, size_t> > &free_list, size_t prev, size_t start);
> -    bool fits_in(vector <pair<size_t, size_t> > &free_list,
> -		 size_t base, Cases& cases);
> -    void insert_state(vector <pair<size_t, size_t> > &free_list,
> -		      State *state, DFA& dfa);
> +	typedef vector <pair <const State *, size_t> >DefaultBase;
> +	typedef vector <pair <const State *, const State *> >NextCheck;
> +      public:

I feel less strong about the public|private keywords being in column 0
(mostly because my thoughts on well formatted C++ are even less
well-formed), my inclination would be to leave them in column 0.

And can I have my bikeshed green with yellow stripes?

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20110311/df893f15/attachment.pgp>