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

[BIND 10 Development]

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

 * owner:  vorner => muks


Comment:

 Hello

 Replying to [comment:4 muks]:
 > * Is there a reason why the reply codes in `proto_defs.cc` are not of
 >   enum type (with explicit values)? At least inside the C++ code, it may
 >   allow some additional type safety, and it can be assigned back from
 >   `intValue()` of an `Element`.

 Well, for one, it is from a different branch. And there are other reasons:
  * The list of constants is not exhaustive. With enum, you implicitly
 claim there's no other value than what is listed, it is not the case here.
 While we don't use anything besides -1, 0 and 1 now, even the 1 is not in
 the constants.
  * Many of the other constants are strings. That wouldn't work for them.
 And I wanted to keep it similar.
  * I want to keep the conversion to python as simple as possible.
  * Can enums handle negative numbers? I believe enums are internally
 unsigned integrals.

 And, besides, you get nearly no type safety if you cast from int to the
 enum or back.

 > * 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.

 > * 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.

 > * 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.

 > * 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).

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