[apparmor] [PATCH 06/10] Lindent + some hand cleanups hfa

[Seth Arnold]

On Thu, Mar 10, 2011 at 12:35 PM, John Johansen
<john.johansen at canonical.com> wrote:
> @@ -124,15 +127,13 @@ void DFA::update_state_transitions(NodeMap &nodemap,
>        }
>  }
>
> -
>  /* WARNING: This routine can only be called from within DFA creation as
>  * the nodes value is only valid during dfa construction.
>  */
>  void DFA::dump_node_to_dfa(void)
>  {
>        cerr << "Mapping of States to expr nodes\n"
> -               "  State  <=   Nodes\n"
> -               "-------------------\n";
> +           "  State  <=   Nodes\n" "-------------------\n";

I prefer the original layout. Would the original persist if each line had the
<< operator prepended?

> @@ -140,7 +141,7 @@ void DFA::dump_node_to_dfa(void)
>  /**
>  * Construct a DFA from a syntax tree.
>  */
> -DFA::DFA(Node *root, dfaflags_t flags) : root(root)
> +DFA::DFA(Node *root, dfaflags_t flags): root(root)
>  {
>        dfa_stats_t stats = { 0, 0, 0 };
>        int i = 0;
> @@ -162,8 +163,8 @@ DFA::DFA(Node *root, dfaflags_t flags) : root(root)
>
>        NodeMap nodemap;
>        NodeSet *emptynode = new NodeSet;
> -       nonmatching = add_new_state(nodemap,
> -                                 make_pair(hash_NodeSet(emptynode), emptynode),
> +       nonmatching = add_new_state(nodemap, make_pair(hash_NodeSet(emptynode),
> +                                                      emptynode),
>                                    emptynode, stats);

I prefer the original layout for this, too.

> @@ -180,12 +181,15 @@ DFA::DFA(Node *root, dfaflags_t flags) : root(root)
>         *       manner, this may help reduce the number of entries on the
>         *       work_queue at any given time, thus reducing peak memory use.
>         */
> -       list<State *> work_queue;
> +       list < State * >work_queue;

Ooof. :)

>        cerr << "Unique Permission sets: " << s << " (" << uniq.size() << ")\n";
>        cerr << "----------------------\n";
> -       for (set< pair<uint32_t, uint32_t> >::iterator i = uniq.begin();
> +       for (set <pair <uint32_t, uint32_t> >::iterator i = uniq.begin();
>             i != uniq.end(); i++) {
> -               cerr << "  " << hex << i->first << " " << i->second << dec <<"\n";
> +               cerr << "  " << hex << i->first << " " << i->
> +                   second << dec << "\n";

I prefer the original layout, or choosing your own line break point. Breaking
across a pointer-member dereference feels icky.

>  /* test if two states have the same transitions under partition_map */
> -bool DFA::same_mappings(State *s1, State *s2)
> +bool DFA::same_mappings(State * s1, State * s2)
>  {

This seems strange.

> -size_t DFA::hash_trans(State *s)
> +size_t DFA::hash_trans(State * s)

And again here.

>                    (partitions.size() % 1000 == 0))
> -                       cerr << "\033[2KMinimize dfa: partitions " << partitions.size() << "\tinit " << partitions.size() << " (accept " << accept_count << ")\r";
> +                       cerr << "\033[2KMinimize dfa: partitions " <<
> +                           partitions.size() << "\tinit " << partitions.
> +                           size() << " (accept " << accept_count << ")\r";

Breaking the line across member access feels icky.

> @@ -454,16 +467,22 @@ void DFA::minimize(dfaflags_t flags)
>                                        (*m)->partition = new_part;
>                                }
>                        }
> -               if ((flags & DFA_DUMP_PROGRESS) &&
> -                   (partitions.size() % 100 == 0))
> -                       cerr << "\033[2KMinimize dfa: partitions " << partitions.size() << "\tinit " << init_count << " (accept " << accept_count << ")\r";
> +                       if ((flags & DFA_DUMP_PROGRESS) &&
> +                           (partitions.size() % 100 == 0))
> +                               cerr << "\033[2KMinimize dfa: partitions " <<
> +                                   partitions.
> +                                   size() << "\tinit " << init_count <<
> +                                   " (accept " << accept_count << ")\r";
>                }

Here as well.

> +                       cerr <<
> +                           "\033[2KDfa minimization no states removed: partitions "
> +                           << partitions.
> +                           size() << "\tinit " << init_count << " (accept " <<
> +                           accept_count << ")\n";

Here as well.

> -               cerr << "\033[2KMinimized dfa: final partitions " << partitions.size() << " (accept " << final_accept << ")" << "\tinit " << init_count << " (accept " << accept_count << ")\n";
> -
> -
> +               cerr << "\033[2KMinimized dfa: final partitions " << partitions.
> +                   size() << " (accept " << final_accept << ")" << "\tinit " <<
> +                   init_count << " (accept " << accept_count << ")\n";

It's consistent! :)

> +       for (Partition::iterator i = states.begin(); i != states.end(); i++) {
> +               if (*i == start || (*i)->accept) {
> +                       os << **i;
> +                       if (*i == start)
> +                               os << " <==";
> +                       if ((*i)->accept) {
> +                               os << " (0x" << hex << (*i)->
> +                                   accept << " " << (*i)->audit << dec << ')';

Very consistent. :)

> +       for (Partition::iterator i = states.begin(); i != states.end(); i++) {
> +               if ((*i)->cases.otherwise)
> +                       os << **i << " -> " << (*i)->cases.otherwise << endl;
> +               for (Cases::iterator j = (*i)->cases.begin();
> +                    j != (*i)->cases.end(); j++) {
> +                       os << **i << " -> " << j->second << ":  " << j->
> +                           first << endl;

And I consistently don't like its choices. :)

> +                       else {
> +                               os << "\t\"" << **i << "\" -> \"";
> +                               os << j->second << "\" [" << endl;
> +                               os << "\t\tlabel=\"" << j->
> +                                   first << "\"" << endl;

Ding!

> +                               if (x.second)
> +                                       class_used = true;
> +                               pair <map <uchar, Chars>::iterator, bool> y =
> +                                   node_classes.
> +                                   insert(make_pair(x.first->second, Chars()));

This bugs me, but not as much as the previous ones.

> -void dump_equivalence_classes(ostream& os, map<uchar, uchar>& eq)
> +void dump_equivalence_classes(ostream & os, map <uchar, uchar> &eq)

It did the right thing with &eq, I wonder why it did & os.

> -    perms |= exact_match_perms &
> -           ~(AA_USER_EXEC_TYPE | AA_OTHER_EXEC_TYPE);
> +       perms |= exact_match_perms & ~(AA_USER_EXEC_TYPE | AA_OTHER_EXEC_TYPE);

I think the original is easier to read.

Thanks John!