BIND 10 #447: MemoryZone::find()

[BIND 10 Development]

#447: MemoryZone::find()
------------------------------+---------------------------------------------
      Reporter:  vorner       |        Owner:  hanfeng  
          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            |  
------------------------------+---------------------------------------------
Changes (by vorner):

  * owner:  vorner => hanfeng


Comment:

 Replying to [comment:7 hanfeng]:
 >   - Since each node in RBTree store the relative name, so the function
 > {{{
 > getRelativeName()
 > }}}

 Yes, I removed it.

 >   check, which also means RBNode is empty, this make the RBNode state
 not clear. Now we don't    have delete for RBTree and MemoryZone, and the
 implementation of MemoryZone should guarantee that  if DOMAIN is empty,
 the data of the node should empty instead with empty map left.

 OK, I added a note about strong exception safety and removed the hack with
 empty map. This is maybe slightly more complicated, but the biggest
 argument is probably mentioned in previous comment (I wasn't aware of need
 to store empty domains, but if it is needed for something, than let's have
 them).

 > trivial coding style
 >   - The exception for each module is declared at the begining which make
 the code reading much easier, but Memory Zone violate it.

 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
 OtherClassBadCondition. And, in generated documentation, you have them
 noted on the same page as the class itself, which is better than searching
 the class list for similar names.

 In short, I think the exceptions should be in the class scope. I put them
 near the function that throws them, but I'm OK with putting them at the
 top of the class (but there's a written rule that constructors should go
 first, so below them). Anyway, I'm not sure we had any codestyle rule
 about position of exception classes. If you think this should be
 different, we should bring this to wider attention.

 >   - For function like getRelativeName, if we really need it, since it's
 a tool function, it's better to be extracted from class definition or
 declare as static function.

 It was out of the public class (and is removed now anyway), so hidden from
 API. This way, as member function, it had access to member data, which it
 used and it was cleaner as where it belongs. I consider that more readable
 and more the OOP way. If it was inside public interface (the .h file), I
 would see the point, but inside the cpp/pimpl class?

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