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

 Here is my review:

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

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

 Same with this too:
 {{{
 +    const ConstElementPtr &result(parseAnswer(rcode, answer));
 }}}

 * We can make `rcode_` inside `RPCError` exception object `const`, as we
   don't expect it to be modified once constructed.

 * A minor nitpick.. we discussed that we'd add spacing between methods a
   few weeks ago:
 {{{
 +    RPCError(const char* file, size_t line, const char* what, int rcode)
 :
 +        CCSessionError(file, line, what),
 +        rcode_(rcode)
 +    {}
 ++ // newline here
 +    /// \brief The error code for the error.
 +    int rcode() const {
 +        return (rcode_);
 +    }
 }}}

 * Not specific to this bug, but there appear to be no unittests for
   `groupSendMsg()`, `groupRecvMsg()`, etc.

 * 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)`?

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