BIND 10 #383: Generalize IOService constructor
BIND 10 Development
do-not-reply at isc.org
Fri Oct 22 07:51:36 UTC 2010
#383: Generalize IOService constructor
------------------------------+---------------------------------------------
Reporter: jelte | Owner: each
Type: enhancement | Status: reviewing
Priority: major | Milestone:
Component: recurser | Resolution:
Keywords: | Sensitive: 0
Estimatedhours: 0.0 | Hours: 0
Billable: 1 | Totalhours: 0
Internal: 0 |
------------------------------+---------------------------------------------
Changes (by jelte):
* owner: jelte => each
* status: new => reviewing
Comment:
Replying to [comment:3 each]:
>
> Review notes:
> {{{
> + /// These are currently very specific to the authoritative server
> + /// implementation.
> }}}
>
> I hadn't realized this comment was still there; it's not correct
anymore.
>
Removed. While I was at it, I also added \param documentation in
asio_link.h
> Not sure why DNSService returns a native asio::io_service instead of an
IOService, but it's fine with me either way. (If it changes, though, it
ought to be renamed getIOService(), both for clarity and for style
compliance. I assume IOService::get_io_service() was named the way it was
because proper capitalization would be misleading about its return type;
anyway, that's why I left it that way.)
>
> asiolink_unittest.cc:
> {{{
> + if (dns_service_ != NULL) {
> + delete dns_service_;
> }
> }}}
>
Hmm. At this moment I think the only reason this function is used is to
get to the native one in the end, which is why right now I made it pass
that through directly. I'm inclined to leave it that way for now. Should
be a trivial change if anyone disagrees.
> Stephen mentioned in his review of #327 that checking for null around a
delete isn't necessary; I forgot to fix that.
>
doh, I knew that. Still some C habits shining through :)
> main.cc:
> {{{
> + io_service = new IOService();
> }}}
>
> io_service could be allocated on the stack now. The only reason it
wasn't before was that it needed to be instantiated down inside the if
statement.
>
Ack. Still needs to be global though, unfortunately.
> recursor_unittest.cc:
> Changes are fine, but will also need to be made in auth_unittest.cc; I
don't see that in the diff.
IOService isn't used directly in those tests, so there wasn't much to
change...
Review comments addressed in r3312
--
Ticket URL: <https://bind10.isc.org/ticket/383#comment:4>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list