SINGA-21 Code review

review singleton.h factory.h
  -- refine singleton to make it thread safe
  -- make factory functions inlined
  -- reformat


Project: http://git-wip-us.apache.org/repos/asf/incubator-singa/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-singa/commit/b0a5832b
Tree: http://git-wip-us.apache.org/repos/asf/incubator-singa/tree/b0a5832b
Diff: http://git-wip-us.apache.org/repos/asf/incubator-singa/diff/b0a5832b

Branch: refs/heads/master
Commit: b0a5832b5d4318605ebf347a6e5b68fb60a93c1e
Parents: 2586e14
Author: wang sheng <[email protected]>
Authored: Mon Jun 22 18:43:42 2015 +0800
Committer: wang wei <[email protected]>
Committed: Wed Jun 24 17:02:52 2015 +0800

----------------------------------------------------------------------
 include/communication/msg.h |  2 +-
 include/utils/factory.h     | 56 +++++++++++++++++++---------------------
 include/utils/singleton.h   | 18 ++++++-------
 3 files changed, 37 insertions(+), 39 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-singa/blob/b0a5832b/include/communication/msg.h
----------------------------------------------------------------------
diff --git a/include/communication/msg.h b/include/communication/msg.h
index 6ff887f..c3ef1c7 100644
--- a/include/communication/msg.h
+++ b/include/communication/msg.h
@@ -4,8 +4,8 @@
 // TODO(wangwei): make it a compiler argument
 #define USE_ZMQ
 
-#include <algorithm>
 #include <string>
+#include <utility>
 
 #ifdef USE_ZMQ
 #include <czmq.h>

http://git-wip-us.apache.org/repos/asf/incubator-singa/blob/b0a5832b/include/utils/factory.h
----------------------------------------------------------------------
diff --git a/include/utils/factory.h b/include/utils/factory.h
index c8fef32..3201853 100644
--- a/include/utils/factory.h
+++ b/include/utils/factory.h
@@ -1,57 +1,55 @@
-#ifndef INCLUDE_UTILS_FACTORY_H_
-#define INCLUDE_UTILS_FACTORY_H_
-#include <glog/logging.h>
+#ifndef SINGA_UTILS_FACTORY_H_
+#define SINGA_UTILS_FACTORY_H_
 
+#include <glog/logging.h>
 #include <functional>
-#include <utility>
 #include <map>
+#include <string>
+
 /**
- * macro that creats a function which instantiate a subclass instance and
+ * Macro that creats a function which instantiate a subclass instance and
  * returns pointer to the base class.
  */
 #define CreateInstance(SubClass, BaseClass) \
   [](void)->BaseClass* {return new SubClass();}
 
 /**
- * factory template to generate class (or a sub-class) object  based on id.
+ * Factory template to generate class (or a sub-class) object based on id.
  * 1. register class creation function that generates a class
  * object based on id.
  * 2. call Create() func to call the creation function and return
  * a pointer to the base calss.
  */
-
 template<typename T>
-class Factory{
- //template<Factory<T>> friend class Singleton;
+class Factory {
  public:
   /**
    * Register functions to create user defined classes.
    * This function is called by the REGISTER_FACTORY macro.
-   * @param id identifier of the creating function/class
-   * @param create_function a function that creates a layer instance
+   * 
+   * @param id Identifier of the creating function/class
+   * @param func a function that creates a layer instance
    */
-  void Register(const std::string id, std::function<T*(void)> func);
+  inline void Register(const std::string& id,
+                       const std::function<T*(void)>& func) {
+    CHECK(str2func_.find(id) == str2func_.end())
+      << "The id has been registered by another function";
+    str2func_[id] = func;
+  }
   /**
-   * create a layer  instance by providing its type
-   * @param type the identifier of the layer to be created
+   * create a layer instance by providing its type
+   * 
+   * @param id The identifier of the layer to be created
    */
-  T *Create(const std::string id);
+  inline T* Create(const std::string& id) {
+    CHECK(str2func_.find(id) != str2func_.end())
+      << "The creation function for " << id << " has not been registered";
+    return str2func_[id]();
+  }
 
  private:
-  //<! Map that stores the registered creation functions
+  // Map that stores the registered creation functions
   std::map<std::string, std::function<T*(void)>> str2func_;
 };
 
-template<typename T>
-void Factory<T>::Register(const std::string id,
-                                        std::function<T*(void)> func) {
-  str2func_[id] = func;
-}
-
-template<typename T>
-T *Factory<T>::Create(const std::string id) {
-  CHECK(str2func_.find(id) != str2func_.end())
-      << "The creation function for " << id << " has not been registered";
-  return str2func_[id]();
-}
-#endif // INCLUDE_UTILS_FACTORY_H_
+#endif  // SINGA_UTILS_FACTORY_H_

http://git-wip-us.apache.org/repos/asf/incubator-singa/blob/b0a5832b/include/utils/singleton.h
----------------------------------------------------------------------
diff --git a/include/utils/singleton.h b/include/utils/singleton.h
index 3c2022b..5048266 100644
--- a/include/utils/singleton.h
+++ b/include/utils/singleton.h
@@ -1,16 +1,16 @@
-#ifndef INCLUDE_UTILS_SINGLETON_H_
-#define INCLUDE_UTILS_SINGLETON_H_
+#ifndef SINGA_UTILS_SINGLETON_H_
+#define SINGA_UTILS_SINGLETON_H_
+
 /**
-  * thread-safe implementation for C++11 according to
+  * Thread-safe implementation for C++11 according to
   * 
http://stackoverflow.com/questions/2576022/efficient-thread-safe-singleton-in-c
   */
 template<typename T>
 class Singleton {
  public:
-
   static T* Instance() {
-    static T* data_=new T();
-    return data_;
+    static T data_;
+    return &data_;
   }
 };
 
@@ -23,9 +23,9 @@ template<typename T>
 class TSingleton {
  public:
   static T* Instance(){
-    static thread_local T* data_=new T();
-    return data_;
+    static thread_local T data_;
+    return &data_;
   }
 };
 
-#endif // INCLUDE_UTILS_SINGLETON_H_
+#endif  // SINGA_UTILS_SINGLETON_H_

Reply via email to