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

[John Johansen]

On 03/11/2011 09:02 PM, Seth Arnold wrote:
> 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?
> 
heck if I know I fought with lindent and lost, I should have just started
from scratch and done it all by hand, I think in the end it might have been
faster

>> @@ -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.
> 
heh, I can understand that, and am more than willing to oblige, this was more
about making the mishmash of styles some what consistent, than following lindent
exactly

>> @@ -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. :)
> 
dang another one I missed, I caught most of them honest


>>        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.
> 
yep, thats 2 votes (besides myself for breaking the 80 column limit will fix)

>>  /* 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.
> 
missed that one too

>> -size_t DFA::hash_trans(State *s)
>> +size_t DFA::hash_trans(State * s)
> 
> And again here.
> 
and again

>>                    (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.
> 
okay

>> @@ -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.
> 
yep

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

>> -               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! :)
> 
yep :D

>> +       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. :)
> 
ah, I can't do it anymore, its more consistent than me

>> +       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. :)
> 
yeah

>> +                       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.
> 
haha, that hurts, I do agree mostly though and will take another pass and cleaning
these up