BIND 10 #2768: C++ part of #2676

[BIND 10 Development]

#2768: C++ part of #2676
-------------------------------------+-------------------------------------
            Reporter:  vorner        |                        Owner:
                Type:  enhancement   |  vorner
            Priority:  medium        |                       Status:
           Component:  Inter-module  |  reviewing
  communication                      |                    Milestone:
            Keywords:                |  Sprint-20130319
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DNS           |                 CVSS Scoring:
Estimated Difficulty:  2.5           |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => vorner


Comment:

 Hi Michal

 Replying to [comment:5 vorner]:
 > And, besides, you get nearly no type safety if you cast from int to the
 enum or back.

 I meant this in the C++ code alone, after the point where you convert it
 from an `Element`. But nod for all the other points.

 > > * Is the & correct in this context:
 > > {{{
 > > +    const ConstElementPtr &command_el(createCommand(command,
 params));
 > > }}}
 > >
 > > As I understand it, it will assign a reference to `command_el`, but
 will
 > > it keep the `ConstElementPtr` object returned by `createCommand()`
 alive
 > > after this statement executes, to use the `ConstElementPtr&` later?
 >
 > Yes. This was suggested to me by Jinmei and I worried the same. I
 > don't know which ticket it was, but he cited part of the
 > standard. There are some occasions when the object is held alive for
 > longer and when you take a return value into a local reference, it is
 > kept alive until the end of the block with the local variable. Yes, it
 > is some kind of exception, but it saves a copy, so I thought it is a
 > nice trick to use.

 I had `libdns++` unittest code crash on a different platform due to
 exactly such code. Its destructor had been called. It had worked
 perfectly on my workstation before. I believe
 `26401e4e861462c03f689c32f97f49d582a64a5b`
 was a commit towards fixing it.

 Can you explain what these occasions are when the object is held alive,
 so I can review whether it is applicable to this situation?  Is it a
 standard C++ behavior, or implementation dependent?

 I think in this place there's no need for such tricks if it could cause
 a problem.

 > > * We can make `rcode_` inside `RPCError` exception object `const`, as
 we
 > >   don't expect it to be modified once constructed.
 >
 > Yes, though in this case, any compiler worth its name should be able
 > to infer the same from the code.

 I don't follow this point. In this case, `const` is intended to protect
 ourselves against accidental modification of this value, when we know it
 must not change after construction. Though this variable is private and
 there is no other member code that touches this variable, we write code
 to describe such things even for future proofing it.

 How does the complier infer and enforce this?

 > > * Not specific to this bug, but there appear to be no unittests for
 > >   `groupSendMsg()`, `groupRecvMsg()`, etc.
 >
 > I did notice. But I don't believe this is the right ticket to fix that
 in.

 Maybe we should create a ticket so we don't forget it. If you agree,
 I'll create one when this ticket comes back to me for review.

 >
 > > * Instead of the convenience version of `rpcCall()`, would rearranging
 > >   the arguments of the other version of `rpcCall()` help by using the
 > >   default arguments, i.e., `rpcCall(command, group, instance, to,
 > >   params)` to `rpcCall(command, group, params, instance, to)`?
 >
 > Well, then you'd have the addressing parameters intermixed with
 > another one and it would be inconsistent with all other methods and
 > with the python version. It seemed cleaner this way. Is there any
 > problem with the convenience overloaded version? I believe it is
 > widely used approach, if not so widely in bind10, then other code uses
 > it a lot (see Qt for example).

 Nod, I follow your reasoning now. I just wondered why you did not
 rearrange the args.

-- 
Ticket URL: <https://bind10.isc.org/ticket/2768#comment:6>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development