BIND 10 #310: proposal: constify lib/cc API more appropriately

[BIND 10 Development]

#310: proposal: constify lib/cc API more appropriately
--------------------------------+-------------------------------------------
      Reporter:  jinmei         |        Owner:  jinmei              
          Type:  enhancement    |       Status:  new                 
      Priority:  minor          |    Milestone:  y2 6 month milestone
     Component:  configuration  |   Resolution:                      
      Keywords:                 |    Sensitive:  0                   
Estimatedhours:  0.0            |        Hours:  0                   
      Billable:  1              |   Totalhours:  0                   
      Internal:  0              |  
--------------------------------+-------------------------------------------

Comment(by jinmei):

 branches/trac310 is ready for review.

 I believe Jelte is the best person to review it, so I'm giving it to him.
 Please speak up if you are too busy or if you can have a suggested
 alternative reviewer.

 I know the diff is large, but most of the changes are trivial type change
 from non-const to const (i.e. safer move), and the fact it compiles and
 passes tests should be sufficient to check the correctness.

 Here are notes on major non trivial points in the diff.  Note that a few
 of them are a fix to bugs of the current code, which should be fixed
 whether or not we adopt this branch.  See 'related notes" on
 updateRemoteConfig() and the bullet for ListElement::set().

  - there are cases where I needed to do more than simple type conversion.
 Most of them are in ccsession.cc.  Relatively trivial ones are changes
 like that for createAnswer():
 {{{
 -    ElementPtr answer_content = answer->get("result");
 +    ElementPtr answer_content = Element::createList();
      answer_content->add(Element::create(rcode));
      answer_content->add(arg);
 +    answer->set("result", answer_content);
 }}}
  that is, since get() is now a const member function that returns a
 !ConstElementPtr object, we cannot use its result to override the content.
 Instead, I begain with a new empty list, built the answer in it, and store
 it in the "answer" pointer.  This makes the whole process a bit more
 complicated than the original code, but I believe the safety benefit from
 the fact that get() returns a non-writable object is a good tradeoff.
  - Likewise, in ModuleCCSession::addRemoteConfig() I made this change:
 {{{
 -    ElementPtr new_config = parseAnswer(rcode, answer);
 -    if (rcode == 0) {
 -        rmod_config.setLocalConfig(new_config);
 +    int rcode;
 +    ConstElementPtr new_config = parseAnswer(rcode, answer);
 +    if (rcode == 0 && new_config) {
 +        ElementPtr local_config = rmod_config.getLocalConfig();
 +        isc::data::merge(local_config, new_config);
 +        rmod_config.setLocalConfig(local_config);
 }}}
   new_config is now a !ConstElementPtr object and cannot be set as a local
 config (which is supposed to be modifiable and cannot be a const).  So I
 take the "get local, merge, then set", thereby effectively making a local
 copy of "new_config".  one possible concern here is a copy overhead, and
 other possible issue is that if the intent is to actually share
 "new_config" with some others, making a copy breaks that assumption.  I
 personally saw these okay, but am open to other opinions.
  - a related note: I added a condition before copying the remote config,
 that is, confirming new_config isn't empty.  I believe this additional
 check is necessary regardless of whether we adopt this set of const-
 related changes because otherwise a subsequent operation of
 updateRemoteConfig() would trigger an exception (at that point the stored
 config can be empty, which breaks an assumption of data::merge())
  - another related note: with this change, the very last addRemoteConfig()
 of CCSessionTest::remoteConfig() would trigger an exception.  the current
 implementation does nothing (not even EXPECT_EQ, etc) on this case, but I
 believe it should actually trigger an exception, and I modified the test
 case accordingly.
  - (again) Likewise, I ended up introducing a different version of
 data::removeIdentical() to make a copy for
 ModuleCCSession::handleConfigUpdate().  As a result, this is actually the
 only version of this function that is used.  So, if this approach makes
 sense, we might even obsolete the other version.
  - Not directly related to these set of changes, but I modified argument
 type of some method/functions from 'const !ElementPtr' to 'const
 Element&'.  These include operator<< and operator== defined in
 lib/cc/data.cc.  This change is two-fold:
    - first, 'const !ElementPtr' is unlikely to be what was intended anyway
 (which is one main subject of this ticket)
    - second, even if we change it to !ConstElementPtr, this interface is
 still questionable.  (in my understanding) shared pointer classes are
 generally expected to behave like bare pointers.  so, if we define
 operator== for them, the naturally expected behavior would be to compare
 the stored pointers (actually, shared_ptr defines this operator that way).
 but our customized version of operator== breaks expectation, which would
 lead to counter intuitive behaviors.  I was not sure if this overriding
 was intentional, but didn't think so, because these are basically only
 used internally and in tests, and none of the usage didn't seem to rely on
 this overriding.
  - due to this change I needed to adjust some test cases and internal code
 of data.cc.  but they should be trivial.
  - I changed the argument type of some functions from (Const)!ElementPtr&
 to (Const)!ElementPtr when possible.  The former allows the called
 function to override the pointer itself (not the object that the pointer
 points to) and can be dangerer than the latter in general.  In most of
 usage, however, this doesn't seem to be the intended interface.
  - there's an off-by-one bug in ListElement::set(). I fixed it in this
 branch and added related test cases to Element::!ListElement tests.  This
 bug needs to be fixed anyway.
  - I tried to avoid introducing unrelated changes to this branch, but
 there are still some unrelated cleanups.  Most of them should be trivial.
 One possibly unclear cleanup would be the removal of
 FakeSession::connect()/sendmsg().  I removed them simply because they were
 unused.

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