Opened 8 years ago

Closed 6 years ago

#1366 closed defect (fixed)

asiolink/dummy_io_cb and uncalled methods

Reported by: jreed Owned by: muks
Priority: low Milestone: bind10-1.2-release-freeze
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

I noticed some doxygen mistakes (with wrong paramaters) so I was curious about this.

Here is an initial idea from Stephen from two months ago.

This patch is against a patch I made (so won't apply to master as is.)

diff --git a/src/lib/asiolink/dummy_io_cb.h b/src/lib/asiolink/dummy_io_cb.h
index bcaefe9..90d24a8 100644
--- a/src/lib/asiolink/dummy_io_cb.h
+++ b/src/lib/asiolink/dummy_io_cb.h
@@ -15,6 +15,7 @@
 #ifndef __DUMMY_IO_CB_H
 #define __DUMMY_IO_CB_H
 
+#include <cassert>
 #include <iostream>
 
 #include <asio/error.hpp>
@@ -39,20 +40,30 @@ public:
 
     /// \brief Asynchronous I/O callback method
     ///
-    /// TODO: explain why this method should never be called.
-    /// This should be unused.
+    /// Should never be called, as this class is a convenience class provided
+    /// for instances where a socket is required but it is known that no
+    /// asynchronous operations will be carried out.
     void operator()(asio::error_code)
     {
-        // TODO: log an error if this method ever gets called.
+        // If the function is called, there is a serious logic error in the
+        // program (this class should not be used as the callback class).  As
+        // the asiolink module is too low-level for logging errors, use assert()
+        // to bug-check the program.
+        assert(false);
     }
 
     /// \brief Asynchronous I/O callback method
     ///
-    /// TODO: explain why this method should never be called.
-    /// This should be unused.
+    /// Should never be called, as this class is a convenience class provided
+    /// for instances where a socket is required but it is known that no
+    /// asynchronous operations will be carried out.
     void operator()(asio::error_code, size_t)
     {
-        // TODO: log an error if this method ever gets called.
+        // If the function is called, there is a serious logic error in the
+        // program (this class should not be used as the callback class).  As
+        // the asiolink module is too low-level for logging errors, use assert()
+        // to bug-check the program.
+        assert(false);
     }
 };
 

Today, Stephen said: Looking at that code again, throwing an exception - rather than using an assert - would be better.

Opening a ticket to take care of this.

Subtickets

Change History (7)

comment:1 Changed 8 years ago by shane

  • Milestone changed from New Tasks to Year 3 Task Backlog

comment:2 Changed 6 years ago by muks

  • Milestone set to bind10-1.2-release-freeze
  • Owner set to UnAssigned
  • Status changed from new to reviewing

Up for review. No ChangeLog entry is necessary.

comment:3 Changed 6 years ago by kean

  • Owner changed from UnAssigned to kean

comment:4 Changed 6 years ago by kean

  • Owner changed from kean to muks

While the change looks fine and doesn't break any existing tests, there is no test for the change itself, and it strikes me that there should be.

comment:5 Changed 6 years ago by muks

  • Owner changed from muks to kean

Tests have been added now. Please review.

comment:6 Changed 6 years ago by kean

  • Owner changed from kean to muks

This now looks fine, please go ahead and merge and close.

comment:7 Changed 6 years ago by muks

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged to master branch in commit 5ff46cb07dfa9f6b373ea9466d7675efa320d926:

* 477969e [1366] Fix copyright year in file
* ff0a309 [1366] Add unittests for DummyIOCallback
* 5dfa164 [1366] Throw an exception if DummyIOCallback() is called
* dcd06d5 [1366] Update doc of dummy_io_cb.h

Resolving as fixed. Thank you for the reviews Kean.

Note: See TracTickets for help on using tickets.