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:  closed              
      Priority:  minor          |    Milestone:  y2 6 month milestone
     Component:  configuration  |   Resolution:  fixed               
      Keywords:                 |    Sensitive:  0                   
Estimatedhours:  0.0            |        Hours:  0                   
      Billable:  1              |   Totalhours:  8.0                 
      Internal:  0              |  
--------------------------------+-------------------------------------------
Changes (by jinmei):

  * status:  reviewing => closed
  * resolution:  => fixed


Comment:

 Replying to [comment:3 jelte]:

 Thanks for the review.

 > I disagree with the statement that the Element API is complicated
 (unless you are talking about the ccsession/modulespec api) btw :p
 >
 Okay, I admit "complicated" is often subjective:-)

 > Two things though; The operator overrides using ElementPtr instead of
 Element were intentional, since users of this API should only be working
 with Ptr's anyway, and to me it looks better to use them directly (compare
 the changed test cases for instance). However I don't have a strong
 opinion on this.
 >
 Hmm, (noting it's not a strong opinon of yours) if you still like the
 previous approach I don't oppose to overriding it.

 Just to be a bit clearer about my comment, here's one specific problematic
 case with the original definition that I can think of:

 {{{
     void some_func(Element* a, Element* b) {
         if (a == b) {
             a->setValue(10);
         }
     }
 }}}

 from this code, we'd expect b's value is also modified to '10', and it's
 quite likely to be the case.  I'd expect the same with the shared pointer
 version of code:

 {{{
     void some_func_shptr(ElementPtr a, ElementPtr b) {
         if (a == b) {
             a->setValue(10);
         }
     }
 }}}

 but this is not always the case with the previous definition of
 operator==, and that's what I meant by "counter intuitive behaviors" in my
 original comment.

 > Secondly, I do worry a bit about the now-introduced copy overhead (which
 was why the merge etc were done in-place). But I acknowledge the added
 safety. So for now I think this is ok. At some point in the future we
 might need to consider having non-const versions of some methods, but we'd
 need to profile those cases when they arise, and for the near future I see
 no problems with them,

 Fair enough.  I'm okay with revisiting this part if/when the copy overhead
 becomes a significant performance bottleneck.

 One more point: I've noticed ConfigData::getModuleSpec() should probably
 return 'const ModuleSpec&' instead of 'const ModuleSpec' and made that
 change (at the very least returning a 'const object' is meaningless
 anyway).  With the current definition of ConfigData these two versions
 should have exactly the same effect, but if the original intent was in
 fact to return a copy of the object, please make that change separately.

 I've merged trac #310 to trunk, and I'm going to close this ticket.

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