BIND 10 #2292: eliminate const_cast from domaintree.h

[BIND 10 Development]

#2292: eliminate const_cast from domaintree.h
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20121009
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:  data   |           Sub-Project:  DNS
  source                             |  Estimated Difficulty:  4
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:6 vorner]:

 > It doesn't seem to be possible. Sometimes, we need the nodes to be
 mutable:
 >  * The `ZoneTable::setZoneData` is quite natural place for a mutable
 node.
 >  * Even the mutability of the `zone_data` in the `FindResult` is used
 sometimes, for example here:
 > {{{#!c++
 > if (fr.zone_data != NULL) {
 >     ZoneData::destroy(mem_sgmt_, fr.zone_data, rrclass_);
 > }
 > }}}

 Actually, I thought this was misuse of the read-only interface and
 wanted to correct at this opportunity.  If we want to possibly modify
 a found node, we should use insert().

 Regarding `setZoneData()` itself (which is only used within
 `InMemoryClientImpl::load()`) I also thought it's badly designed and
 used (probably as a result of being developed incrementally and by
 multiple developers).  What we should have really done (and will have
 to do IMO) is to pass both zone name and zone data to `addZone()` and
 let it insert the name and set the data (in the current implementation
 `addZone()` creates the zone data itself, but with the latest code
 logic it's mostly meaningless.  I guess this is one of the things we
 didn't do well in the incremental development).  Then we won't even
 need `setZoneData()` at all (at least for the current set of
 features).

 > So if we found a way to get rid of the mutability, I think it would feel
 very
 > unnatural and counter-intuitive.
 >
 > What I'm thinking of is making the find variants that return mutable
 > version as non-const (so you couldn't call them on const domain
 > tree). I think this would need some changes in the tests (I tried
 > something like that as an experiment and it complained), but it
 > should be possible and more or less logical.

 If we inevitably need the immutable version at least in the current
 code logic, I'm okay with that.  But I'm not yet convinced about that;
 I explained we don't need it for the zone table/data example above,
 and in my initial investigation I thought we didn't see inevitable
 need for it.

 Adding a mutable version of things makes the implementation fragile. A
 careless developer could use the mutable version without thinking
 about the necessity, leaving source of bugs; if we only have an
 immutable version we can enforce the safer behavior at compile time.

 So, while I don't oppose to your approach as a general sense, in this
 particular case I'm not yet convinced and would like to seek to begin
 with const-only version.  If future needs require real mutability,
 then I'll accept the interface extension at that point.

 If it's not convincing to you, can I try to see if I can offer a
 counter proposal?  Maybe I'm wrong and there's a real place that
 cannot avoid mutability, but I'd like to be confident about that with
 an actual attempt.

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