[apparmor] [PATCH 07/10] Lindent + some hand cleanups expr-tree

[John Johansen]

On 03/11/2011 06:37 PM, Seth Arnold wrote:
> Thanks John, a few comments inline:
> 
> On Thu, Mar 10, 2011 at 12:35 PM, John Johansen
> <john.johansen at canonical.com> wrote:
>> @@ -249,7 +246,6 @@ static Node *merge_charset(Node *a, Node *b)
>>                        to->insert(*i);
>>                return b;
>>        }
>> -
>>        //return ???;
>>  }
> 
> It really would be nice to know more about this. :)
> 
well its dead code that I didn't change,

longer version its code I was writing to do char set merging and splitting
but I ran out of time to finish it up.  I left it in place to come back
to it another time, and its sat since.

And now, well I have a series of patches that completely reworks how the
tree simplification works so the code will be dropped as soon as I can
finish up that series.

>>  typedef unsigned char uchar;
>> -typedef set<uchar> Chars;
>> +typedef set <uchar> Chars;
> 
> This behavior is strange. I'm much more used to the set<uchar> style.
> 
gah, /me swears of lindent

>> -ostream& operator<<(ostream& os, uchar c);
>> +ostream & operator<<(ostream &os, uchar c);
>>
>>  /* Compute the union of two sets. */
>> -template<class T>
>> -set<T> operator+(const set<T>& a, const set<T>& b)
>> +template <class T> set <T> operator+(const set <T> &a, const set <T> &b)
>>  {
>> -       set<T> c(a);
>> +       set < T > c(a);
> 
> .. and here it is an utter failure. :) Is there a different Lindent mode
> for handling C++ templates?
> 
well there are multiple indent modes but it pretty bad, I just chose lindent
because its the style that we settled on long ago.  I would be more than
willing to revist it that decision :)

>> @@ -83,27 +82,33 @@ ostream& operator<<(ostream& os, const NodeSet& state);
>>  * enumerating all the explicit tranitions for default matches.
>>  */
>>  typedef struct NodeCases {
>> -       typedef map<uchar, NodeSet *>::iterator iterator;
>> +       typedef map <uchar, NodeSet *>::iterator iterator;
> 
> Similar problem here.
> 
yep thanks

>>        iterator begin() { return cases.begin(); }
>>        iterator end() { return cases.end(); }
>>
>> -       NodeCases() : otherwise(0) { }
>> -       map<uchar, NodeSet *> cases;
>> +       NodeCases(): otherwise(0) { }
>> +       map <uchar, NodeSet *> cases;
> 
> And here. :)
> 
..

>> @@ -340,10 +337,10 @@ public:
>>  };
>>
>>  /* Match any character (/./). */
>> -class AnyCharNode : public CNode {
>> -public:
>> +class AnyCharNode:public CNode {
>> +      public:
>>        AnyCharNode() { }
>> -       void follow(NodeCases& cases)
>> +       void follow(NodeCases & cases)
> 
> Hrm, elsewhere the ampersand was placed next to the parameter, which I
> prefer; the spaces on both sides are strange. :)
> 
ah thats because I manually fixed those up, missed this one

>> @@ -385,40 +380,32 @@ public:
>>        /* requires accept nodes to be common by pointer */
>>        int eq(Node *other)
>>        {
>> -               if (dynamic_cast<AcceptNode *>(other))
>> +               if (dynamic_cast < AcceptNode * >(other))
> 
> The other instances of replacing
>     dynamic_cast<foo *>
> with
>     dynamic_cast <foo *>
> didn't bother me much, but this looks too funny.
> 
yes it does sorry, will fix

>> @@ -427,36 +414,26 @@ public:
> ...
>> +class PlusNode:public OneChildNode {
>> +      public:
>> +       PlusNode(Node *left): OneChildNode(left) {
>>        }
> 
> Hunh, I'm surprised this is on two lines. It'd be more obviously empty
> if both brackets were on the same line.
> 
gah, I really thought I got all those, but I guess after staring at all
the lindent breakage after a while its all to easy to miss a few