Return to Answer

I will try not to give awful advice like I did last time. Here are a few remarks:

• First of all, let's come back to std::forward. You should follow the link and read again a little bit more; std::forward is only useful for reference collapsing. In your operator(), you take the arguments by copy (that is fine) and there is no reference collapsing. Therefore, where you use std::forward, you should use std::move instead (from the header <algorithm>), following the copy-then-move idiom.

• You can simplify your default constructor. First of all, you can use in-class initializers to initialize connections_ and blocked_:

   connection_p connections_ = nullptr;
   bool blocked_ = false;
  

  Now, if the values to pass to connections_ and blocked_ are not specified in a constructor, the compiler will use the values from the in-class initializers instead. That means that you can reduce the implementation of Signal's default constructor to:

   Signal() = default;
  

• There are too many comments. Try to remove the unneeded comments. Many of your comments do not give more information that what we already know by reading the corresponding line of code. Even worse, if you modify the code and not the comments, they will live. Try to writer self-documenting code to avoid unneeded and potentially liar comments.

• However, your comments helped me figure out that you need a C++11 feature here:

   private:
   /** don't allow copy construction **/
   Connection(const Connection& other);
   /** don't allow copy assignment **/
   Connection& operator= (Connection& other);
  

  Instead of making these special functions private, you can explicitely mark them as deleted. In other words, you can write this instead:

   Connection(const Connection& other)
   = delete;
   Connection& operator= (Connection& other)
   = delete;
  

  And the same holds for copy assignment operator of Signal.

• Thanks @Jamal for this one: try not to use std::endl when it is not needed. Actually, std::endl adds a newline character and flushes the output buffer. You generally don't need the buffer to be flushed, you only want to add a newline character. Moreover, the buffer is automatically flushed when the program ends.

• There are many places where you can still use list initialization to avoid potential implicit narrowing conversion. I already mentioned that in my answer to your previous question and forgot to mention the cases where it was not possible (references for example). A better guideline would actually be to use list initialization when you use numbers (integers of floating point numbers) and you know that a loss of precision could be a problem. You can also use it to prevent implicit conversions to bool, which sometimes don't make sense.

I will try not to give awful advice like I did last time. Here are a few remarks:

• First of all, let's come back to std::forward. You should follow the link and read again a little bit more; std::forward is only useful for reference collapsing. In your operator(), you take the arguments by copy (that is fine) and there is no reference collapsing. Therefore, where you use std::forward, you should use std::move instead (from the header <algorithm>), following the copy-then-move idiom.

• You can simplify your default constructor. First of all, you can use in-class initializers to initialize connections_ and blocked_:

   connection_p connections_ = nullptr;
   bool blocked_ = false;
  

  Now, if the values to pass to connections_ and blocked_ are not specified in a constructor, the compiler will use the values from the in-class initializers instead. That means that you can reduce the implementation of Signal's default constructor to:

   Signal() = default;
  

• There are too many comments. Try to remove the unneeded comments. Many of your comments do not give more information that what we already know by reading the corresponding line of code. Even worse, if you modify the code and not the comments, they will live. Try to writer self-documenting code to avoid unneeded and potentially liar comments.

• However, your comments helped me figure out that you need a C++11 feature here:

   private:
   /** don't allow copy construction **/
   Connection(const Connection& other);
   /** don't allow copy assignment **/
   Connection& operator= (Connection& other);
  

  Instead of making these special functions private, you can explicitely mark them as deleted. In other words, you can write this instead:

   Connection(const Connection& other)
   = delete;
   Connection& operator= (Connection& other)
   = delete;
  

  And the same holds for copy assignment operator of Signal.

• Thanks @Jamal for this one: try not to use std::endl when it is not needed. Actually, std::endl adds a newline character and flushes the output buffer. You generally don't need the buffer to be flushed, you only want to add a newline character. Moreover, the buffer is automatically flushed when the program ends.

• There are many places where you can still use list initialization to avoid potential implicit narrowing conversion. I already mentioned that in my answer to your previous question and forgot to mention the cases where it was not possible (references for example). A better guideline would actually be to use list initialization when you use numbers (integers of floating point numbers) and you know that a loss of precision could be a problem. You can also use it to prevent implicit conversions to bool, which sometimes don't make sense.