BIND 10 #447: MemoryZone::find()

[BIND 10 Development]

#447: MemoryZone::find()
------------------------------+---------------------------------------------
      Reporter:  vorner       |        Owner:  vorner   
          Type:  task         |       Status:  reviewing
      Priority:  major        |    Milestone:           
     Component:  data source  |   Resolution:           
      Keywords:               |    Sensitive:  0        
Estimatedhours:  0.0          |        Hours:  0        
      Billable:  1            |   Totalhours:  0        
      Internal:  0            |  
------------------------------+---------------------------------------------

Comment(by vorner):

 Replying to [comment:9 hanfeng]:
 > > But these exceptions are not module-level. They are class level.
 Nobody else throws them than the class, so the class should contain them.
 I see it better to have names like SomeClass::BadCondition and
 OtherClass::BadCondition than SomeClassBadCondition and [...]
 > Sorry for my inaccurate word, module here means MemoryZone. Just like
 classes, exception also has hierarchy, more specific exception is raised
 by specific class. There is no code guildline for exception class
 definition, but please check the code in src/lib/dns, for example name.h,
 all the exceptions are only needed by name class. Personally, I think
 declare the exception on the top is more clear.

 Ok, I'm going to ask on the mailing list. But there are two kind of
 hierarchies (both for exceptions and other classes) ‒ inheritance
 hierarchy and namespace hierarchy. They are orthogonal. I don't know why I
 shouldn't use both of them.

 And it might more clear/convenient when you already opened the header file
 and started reading the code. But more often, you just open the
 documentation and read the class description, not its header. And having
 the list of related exceptions on the same page (as nested classes) is
 more convenient than them just having same prefix in the name.

 > In
 > {{{
 > result::Result add(const ConstRRsetPtr& rrset)
 > }}}
 > I see you add check to make sure new added rrset should belong to
 current zone, but I am wondering the steps to add rrset is that, at first,
 find suitable zone in datasrc first, then add rrset into the zone, I am
 afraid the same check will be done in two places.

 The check is maybe redundant in current use. But from design point of
 view, the class itself can't make any assumption about who and how it is
 used. Adding the check is just defensive programming. It checks it is used
 in a correct way without relying on external world, and it is there for
 the same reason we have asserts in the code and checks for NULL pointers
 as parameters all over the place, even at places where passing a NULL
 pointer makes no sense. It makes the code more robust and makes debugging
 easier in case of problem.

 And the check was there as part the getRelativeName before, it was just
 implicit.

 About the PARTIALMATCH, currently it means NXDOMAIN always. Bud I added a
 comment warning about it.

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