BIND 10 #977: ACL base class
BIND 10 Development
do-not-reply at isc.org
Thu Jun 9 08:48:19 UTC 2011
#977: ACL base class
-------------------------------------+-------------------------------------
Reporter: | Owner: jinmei
vorner | Status: accepted
Type: task | Milestone:
Priority: major | Sprint-20110614
Component: | Resolution:
Unclassified | Sensitive: 0
Keywords: | Sub-Project: DNS
Defect Severity: N/A | Estimated Difficulty: 0.0
Feature Depending on Ticket: ACL | Total Hours: 0
Add Hours to Ticket: 0 |
Internal?: 0 |
-------------------------------------+-------------------------------------
Comment (by jinmei):
'''General'''
I was not sure if this design is good or not just from this set of
definitions, including
- whether this style of mixture of inheritance and template is the
best or quite better abstraction of what we'd like to achieve
- the fact that having both Check and CompoundCheck are a base class
indicates we'll have quite deep levels of inheritance hierarchy.
without a care such a design can be difficult to manage.
But, not to waste time, I'd like to be clear that these are not
objections. After all, we won't be able to be sure about these points
until we implement more specific cases. So, I'm okay with moving
forward with it for now. You may or may not want to answer these
points at this stage, but I don't request resolving the discussion as
a prerequisite for merge.
'''Specific comments'''
- I made some trivial editorial changes and pushed them directly.
Please check.
- To describe template parameters for Check and CompoundCheck, I'd use
typename (instead of class) if types that are not a "pure" class can
be used as "Context" as is the case in the test. This is basically
a matter of taste, however, and it's up to you.
- maybe it's better to make the constructor of the base class protected.
- s/subexpressions()/getSubexpressions()/ according to our naming
guideline.
- I'd like to have descriptions about possible exceptions, at least
for some important methods such as matches().
- is it intentional that Check::toText() is *not* virtual? If it's
intentional,
the fact that this class has a pure virtual method will cause an
awkward effect, i.e., we'll have to provide the implementation of
the pure virtual method, although it's not prohibited.
- I'd avoid using hardcode for 10000. I'd instead define a class
static constant and refer to it anywhere else.
--
Ticket URL: <http://bind10.isc.org/ticket/977#comment:5>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list