BIND 10 #997: Create the ACL class

[BIND 10 Development]

#997: Create the ACL class
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  vorner                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20110614
                  Component:         |            Resolution:
  Unclassified                       |             Sensitive:  0
                   Keywords:         |           Sub-Project:  Core
            Defect Severity:  N/A    |  Estimated Difficulty:  0.0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jinmei):

 * milestone:  Sprint-20110628 => Sprint-20110614


Comment:

 Replying to [comment:6 vorner]:

 > Replying to [comment:4 jinmei]:
 > > First, I made some trivial editorial changes and pushed them.
 > > Having/removing a white space, e.g. from "{ }" may be purely matter of
 > > taste (and the coding guideline doesn't say anything about this
 > > point), but it seems we don't have a space for these cases in the vast
 > > majority of the code, so I made them consistent.
 >
 > I noticed ‒ I thought we were using camelCase for C++ code and
 underscores in python. Was I mistaken?

 We use the camel for classes, but use (lower case+) underscores for
 variables.  See the "Naming" section of
 http://bind10.isc.org/wiki/CodingGuidelines

 > > - execute(): is it reasonable to return an Action "object"?  This
 > >   means it must be copyable, and, depending on the internal detail of
 > >   the actual Action type, the copy could be very expensive.  On a
 > >   related note, maybe it would be useful to describe the minimal
 > >   requirement (such as copyability) that the Action template parameter
 > >   must meet, and, if it's basically intended to be integral types,
 > >   mention that, too.
 >
 > I'm returning reference to it now (I somehow implicitly expected to be
 something small, but you're right, when we allow it to be quite anything,
 it may be worth it). But it still needs to be copyable, since I need to
 place it into the vector.

 Okay.

 > > - execute(): this type of for loop is a bit more expensive because it
 > >   involves a call to entries_.end() in each iteration.  I'd not
 > >   necessarily request it be addressed right now, but since this could
 > >   be a performance sensitive path, I believe we should be careful.
 >
 > Well, considering that the vector is template, therefore completely
 available to compiler and the function is inlined, if it is combined with
 the fact that vector guarantees being really an array and iterator being
 wrapped pointer, this should optimise to single pointer comparison with
 the end pointer being fetched from constant memory location. Would there
 be any way of avoiding even that?

 I suspect it's not safe to rely on specific compiler optimization too
 much (on the other hand, of course, it wouldn't be wise to introduce
 micro optimization in the code level when compilers are normally
 expected to do a better job.  so it's a trade off issue, and in this
 case I personally think it's better to be careful).  I'd also note
 that it's not guaranteed that iterators are implemented as a pointer.

 There are several ways to avoid the "obvious waste", and one
 straightforward way is to define the end iterator explicitly, outside
 the scope of the loop

 {{{
         typename Entries::const_iterator const i_end(entries_.end());
         for (typename Entries::const_iterator i(entries_.begin());
              i != i_end; ++i) {
 }}}

 When we can rely on smartness of the compiler, the resulting code
 would be equivalent to the most efficient version of the result from
 the original code; when we cannot and/or the iterator is not
 implemented as a pointer, the resulting code of this version will
 still avoid the redundant evaluation of end().

 I suspect you were of course aware of it and didn't choose to do so
 for some reason, however.  Perhaps that was for brevity.  I personally
 think in this case the additional code doesn't damage the readability
 too much, but, as I already said, I wouldn't insist it be included.

 One other minor point(s): if we change "policy" to "default action" in
 acl.h, it would be better to make it consistent in acl_test.c.  For
 example.
  - checkPolicy would be renamed to checkDefaultAction
  - emptyPolicy would be something like emptyRule
  - AclTest::policy would be something like noDefaultAction
  - overall comment update

 Oh, and one really last, very minor thing, maybe you want to rename
 AclTest to ACLTest.

-- 
Ticket URL: <http://bind10.isc.org/ticket/997#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development