[apparmor] [patch 07/12] Move nodeset caching into expr-tree.h

[John Johansen]

On 08/20/2014 11:45 PM, Seth Arnold wrote:
> On Fri, Aug 15, 2014 at 12:20:42PM -0700, john.johansen at canonical.com wrote:
>> We need to rework permission type mapping to nodesets, which means we
>> need to move the nodeset computations earlier in the dfa creation
>> processes, instead of a post step of follow(), so move the nodeset
>> into expr-tree
>>
>> Signed-off-by: John Johansen <john.johansen at canonical.com>
> 
> I asked some questions inline, but since this patch didn't introduce any
> of what I'm curious about:
> 
> Acked-by: Seth Arnold <seth.arnold at canonical.com>
> 

<<snip>>

>> +	NodeSet *insert(NodeSet *nodes)
>> +	{
>> +		if (!nodes)
>> +			return NULL;
>> +		pair<set<hashedNodeSet>::iterator,bool> uniq;
>> +		uniq = cache.insert(hashedNodeSet(nodes));
> 
> Does this make two hashedNodeSet objects, one on the stack and then one
> in the 'cache' set<>? Or does this copy stack object into the set<>? Would
> a shared_ptr or new here only create one?
>
Welcome to the hell of C++ with its copy, assignment and reference constructors
etc.

The first line declares an object that may or may not be built with the null
constructor at the discretion of the compiler, the second assigns it. The
compiler should notice this and not create the first object but, it could
just destroy it instead.

>> +		if (uniq.second == false) {
>> +			delete(nodes);
>> +			dup++;
> 
> Is it expected behaviour for set::insert to delete its argument?
> 
in this case yes, but its not really something that is a standard practice.
This is some of the code that needs to go away. We need to be properly accumulating
these from the beginning as separate sets, instead of resplitting them after the fact.

>> +		} else {
>> +			sum += nodes->size();
>> +			if (nodes->size() > max)
>> +				max = nodes->size();
>> +		}
>> +		return uniq.first->nodes;
>> +	}
>> +};
>> +
>> +struct deref_less_than {
>> +       bool operator()(hashedNodeVec * const &lhs, hashedNodeVec * const &rhs)const
>> +		{
>> +			return *lhs < *rhs;
>> +		}
>> +};
> 
> There's funny whitespace all over this little class -- the 'const' at the
> end of the prototype is missing a space and I'm pretty sure the braces are
> in the wrong column.
>
feel free to provide a ws patch