BIND 10 #3171: PD: AllocationEngine support for Prefix Delegation, pt.2

[BIND 10 Development]

#3171: PD: AllocationEngine support for Prefix Delegation, pt.2
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                Type:  enhancement   |                       Status:
            Priority:  high          |  reviewing
           Component:  dhcp6         |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130918
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => tomek


Comment:

 Replying to [comment:4 tomek]:
 > Replying to [comment:3 marcin]:
 > > Reviewed commit e09660860573d1b76c65a200d3682d3592406b67

 Reviewed commit a019cd162f1ad9584dcc67732ee545c6395bebb4

 I fixed one typo in the doxygen, so please pull these changes.

 > >
 > > Please propose a !ChangeLog entry for this ticket.

 Please! :-)

 > >
 > > '''src/lib/dhcpsrv/alloc_engine.cc'''
 > > * Sorry for the neat pick but both the brief of each increaseAddress
 and increasePrefix should start with the capital letter.
 > There's a lot of other comments that do not start with capital letters.
 Fixed some of those. I'm sorry to ruin your expectations, but I'm afraid
 that we'll have to do a demo with lowercase comments. :P

 I am depressed but I will try to be a big boy and will not cry.

 > > '''src/lib/dhcpsrv/libdhcpsrv.dox'''
 > > * s/At lease 3 allocators will be implemented/At least 3 allocators
 will be implemented/
 > > * I would suggest that the first paragraph of the new section:
 !''Prefix Delegation support in Allocation Engine!'' is moved to a
 separate section. I would just leave the second section which is strictly
 about the PD support which this section should be about.
 > I didn't get that comment. Prefix Delegation support in !AllocEngine
 *is* a separate section.

 Sorry. I meant that the !AllocEngine section consists of two paragraphs
 and the first one could be moved to other section. But, treat it
 suggestion. I don't insist.

 > > '''src/lib/dhcpsrv/subnet.cc'''
 > > * getPools (variable variant): IMHO, the non-const variant would be
 more appropriate.
 > I don't get this comment. There are const and non-const variants.
 Depending on the case both are used.

 Sorry. I wasn't clear enough here. I think the commentary should be
 changed to say:
 {{{
     /// @brief Returns all pools (non-const variant)
 }}}

 The variable variant sounds ambiguous to me.

 >
 > > * The non-const variant of getPools is used internally by the Subnet
 class. In this case, there is no need to make it public. Especially, that
 it leads to the situation that the class members may be modified
 externally when returned via non-const variant of the function. However,
 if you make it private you will end-up having one overload of getPools
 private and another one public which will cause the compilation to fail
 for a different reason: compiler will complain that the getPools is
 private even though you want to access the public (const) variant. I
 believe, that the reasonable way to overcome this problem is to give the
 private function a different name: say getPoolsWritable or something like
 that and use it internally within Subnet class.
 > C++ purity is not a deliverable on the demo. I can add a @todo to the
 code if you like, but it is unlikely that anyone would implement this
 before November.

 It is unlikely to be implemented if we put the todo. It is not an
 extensive change. It is actually very simple and I was able to implement
 it locally within 5 minutes when doing this review. See:

 http://pastebin.com/45R7Ky0C

 I wouldn't say it is a C++ purity, but rather a standard way of coding
 that we struggle to  apply everywhere. If you think it is a hassle, please
 add a @todo.

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