Clean code (jupyter-xeus#43)

martinRenou · SylvainCorlay · commit 8dba75c1443d · 2018-12-06T09:06:55.000+01:00
* Import right builtins module depending on the Python version

* Use const&amp; everywhere

* Compile time Python version check
diff --git a/src/xcomm.cpp b/src/xcomm.cpp
@@ -26,7 +26,7 @@ namespace nl = nlohmann;
 
 namespace xpyt
 {
-    xcomm::xcomm(py::args /*args*/, py::kwargs kwargs)
+    xcomm::xcomm(const py::args& /*args*/, const py::kwargs& kwargs)
         : m_comm(target(kwargs), id(kwargs))
     {
         m_comm.open(
@@ -55,7 +55,7 @@ namespace xpyt
         return true;
     }
 
-    void xcomm::close(py::args /*args*/, py::kwargs kwargs)
+    void xcomm::close(const py::args& /*args*/, const py::kwargs& kwargs)
     {
         m_comm.close(
             kwargs.attr("get")("metadata", py::dict()),
@@ -64,7 +64,7 @@ namespace xpyt
         );
     }
 
-    void xcomm::send(py::args /*args*/, py::kwargs kwargs)
+    void xcomm::send(const py::args& /*args*/, const py::kwargs& kwargs)
     {
         m_comm.send(
             kwargs.attr("get")("metadata", py::dict()),
@@ -73,23 +73,23 @@ namespace xpyt
         );
     }
 
-    void xcomm::on_msg(python_callback_type callback)
+    void xcomm::on_msg(const python_callback_type& callback)
     {
         m_comm.on_message(cpp_callback(callback));
     }
 
-    void xcomm::on_close(python_callback_type callback)
+    void xcomm::on_close(const python_callback_type& callback)
     {
         m_comm.on_close(cpp_callback(callback));
     }
 
-    xeus::xtarget* xcomm::target(py::kwargs kwargs) const
+    xeus::xtarget* xcomm::target(const py::kwargs& kwargs) const
     {
         std::string target_name = kwargs["target_name"].cast<std::string>();
         return xeus::get_interpreter().comm_manager().target(target_name);
     }
 
-    xeus::xguid xcomm::id(py::kwargs kwargs) const
+    xeus::xguid xcomm::id(const py::kwargs& kwargs) const
     {
         if (py::hasattr(kwargs, "comm_id"))
         {
@@ -102,7 +102,7 @@ namespace xpyt
         }
     }
 
-    auto xcomm::cpp_callback(python_callback_type py_callback) const -> cpp_callback_type
+    auto xcomm::cpp_callback(const python_callback_type& py_callback) const -> cpp_callback_type
     {
         return [this, py_callback](const xeus::xmessage& msg) {
             py_callback(cppmessage_to_pymessage(msg));
@@ -117,9 +117,9 @@ namespace xpyt
     {
     }
 
-    void register_target(py::str target_name, py::object callback)
+    void register_target(const py::str& target_name, const py::object& callback)
     {
-        auto target_callback = [target_name, callback] (xeus::xcomm&& comm, const xeus::xmessage& msg) {
+        auto target_callback = [&callback] (xeus::xcomm&& comm, const xeus::xmessage& msg) {
             callback(xcomm(std::move(comm)), cppmessage_to_pymessage(msg));
         };
 
diff --git a/src/xcomm.hpp b/src/xcomm.hpp
@@ -33,24 +33,24 @@ namespace xpyt
         using cpp_callback_type = std::function<void(const xeus::xmessage&)>;
         using zmq_buffers_type = std::vector<zmq::message_t>;
 
-        xcomm(py::args args, py::kwargs kwargs);
+        xcomm(const py::args& args, const py::kwargs& kwargs);
         xcomm(xeus::xcomm&& comm);
         xcomm(xcomm&& comm) = default;
         virtual ~xcomm();
 
         std::string comm_id() const;
         bool kernel() const;
 
-        void close(py::args args, py::kwargs kwargs);
-        void send(py::args args, py::kwargs kwargs);
-        void on_msg(python_callback_type callback);
-        void on_close(python_callback_type callback);
+        void close(const py::args& args, const py::kwargs& kwargs);
+        void send(const py::args& args, const py::kwargs& kwargs);
+        void on_msg(const python_callback_type& callback);
+        void on_close(const python_callback_type& callback);
 
     private:
 
-        xeus::xtarget* target(py::kwargs kwargs) const;
-        xeus::xguid id(py::kwargs kwargs) const;
-        cpp_callback_type cpp_callback(python_callback_type callback) const;
+        xeus::xtarget* target(const py::kwargs& kwargs) const;
+        xeus::xguid id(const py::kwargs& kwargs) const;
+        cpp_callback_type cpp_callback(const python_callback_type& callback) const;
 
         xeus::xcomm m_comm;
     };
diff --git a/src/xdisplay.cpp b/src/xdisplay.cpp
@@ -37,7 +37,7 @@ namespace xpyt
         m_execution_count = execution_count;
     }
 
-    void xdisplayhook::operator()(py::object obj, bool raw = false)
+    void xdisplayhook::operator()(const py::object& obj, bool raw = false) const
     {
         auto& interp = xeus::get_interpreter();
 
@@ -67,7 +67,7 @@ namespace xpyt
         }
     }
 
-    nl::json display_pub_data(py::object obj)
+    nl::json xdisplayhook::display_pub_data(const py::object& obj) const
     {
         py::module py_json = py::module::import("json");
         nl::json pub_data;
diff --git a/src/xdisplay.hpp b/src/xdisplay.hpp
@@ -28,14 +28,14 @@ namespace xpyt
         virtual ~xdisplayhook();
 
         void set_execution_count(int execution_count);
-        void operator()(py::object obj, bool raw);
+        void operator()(const py::object& obj, bool raw) const;
 
     private:
 
+        nl::json display_pub_data(const py::object& obj) const;
+
         int m_execution_count;
     };
-
-    nl::json display_pub_data(py::object obj);
 }
 
 #endif
diff --git a/src/xinput.cpp b/src/xinput.cpp
@@ -17,7 +17,7 @@
 #include "pybind11/functional.h"
 
 #include "xinput.hpp"
-#include "xpyt_config.hpp"
+#include "xutils.hpp"
 
 namespace py = pybind11;
 
@@ -68,37 +68,29 @@ namespace xpyt
          .def("getpass", getpass, py::arg("prompt") = "")
          .def("notimplemented", notimplemented, py::arg("prompt") = "");
 
-        if (PY_MAJOR_VERSION == 2)
-        {
-            m.def("raw_input", input, py::arg("prompt") = "");
-        }
+#if PY_MAJOR_VERSION == 2
+        m.def("raw_input", input, py::arg("prompt") = "");
+#endif
     }
 
     // Implementation of input_redirection
 
-#if PY_MAJOR_VERSION == 2
-#define XEUS_PYTHON_BUILTINS __builtin__
-#else
-#define XEUS_PYTHON_BUILTINS builtins
-#endif
-
     input_redirection::input_redirection(bool allow_stdin)
     {
         py::module xeus_python_input = py::module::import("xeus_python_input");
 
         // Forward input()
-        py::module builtins = py::module::import(XPYT_STRINGIFY(XEUS_PYTHON_BUILTINS));
+        py::module builtins = py::module::import(XPYT_BUILTINS);
         m_sys_input = builtins.attr("input");
         builtins.attr("input") = allow_stdin ? xeus_python_input.attr("input")
                                              : xeus_python_input.attr("notimplemented");
 
+#if PY_MAJOR_VERSION == 2
         // Forward raw_input()
-        if (PY_MAJOR_VERSION == 2)
-        {
-            m_sys_raw_input = builtins.attr("raw_input");
-            builtins.attr("raw_input") = allow_stdin ? xeus_python_input.attr("raw_input")
-                                                     : xeus_python_input.attr("notimplemented");
-        }
+        m_sys_raw_input = builtins.attr("raw_input");
+        builtins.attr("raw_input") = allow_stdin ? xeus_python_input.attr("raw_input")
+                                                 : xeus_python_input.attr("notimplemented");
+#endif
 
         // Forward getpass()
         py::module getpass = py::module::import("getpass");
@@ -110,14 +102,13 @@ namespace xpyt
     input_redirection::~input_redirection()
     {
         // Restore input()
-        py::module builtins = py::module::import(XPYT_STRINGIFY(XEUS_PYTHON_BUILTINS));
+        py::module builtins = py::module::import(XPYT_BUILTINS);
         builtins.attr("input") = m_sys_input;
 
+#if PY_MAJOR_VERSION == 2
         // Restore raw_input()
-        if (PY_MAJOR_VERSION == 2)
-        {
-            builtins.attr("raw_input") = m_sys_raw_input;
-        }
+        builtins.attr("raw_input") = m_sys_raw_input;
+#endif
 
         // Restore getpass()
         py::module getpass = py::module::import("getpass");
diff --git a/src/xinterpreter.cpp b/src/xinterpreter.cpp
@@ -19,6 +19,7 @@
 #include "pybind11/functional.h"
 
 #include "xpyt_config.hpp"
+#include "xutils.hpp"
 #include "xdisplay.hpp"
 #include "xinput.hpp"
 #include "xinterpreter.hpp"
@@ -122,7 +123,7 @@ namespace xpyt
         {
             // Import AST ans builtins modules
             py::module ast = py::module::import("ast");
-            py::module builtins = py::module::import("builtins");
+            py::module builtins = py::module::import(XPYT_BUILTINS);
 
             // Parse code to AST
             py::object code_ast = ast.attr("parse")(code, "<string>", "exec");
diff --git a/src/xutils.cpp b/src/xutils.cpp
@@ -48,7 +48,7 @@ namespace xpyt
         return bufferlist;
     }
 
-    std::vector<zmq::message_t> pylist_to_zmq_buffers(py::list bufferlist)
+    std::vector<zmq::message_t> pylist_to_zmq_buffers(const py::list& bufferlist)
     {
         std::vector<zmq::message_t> buffers;
         for (py::handle buffer : bufferlist)
@@ -130,7 +130,7 @@ namespace nlohmann
             }
         }
 
-        json to_json_impl(py::handle obj)
+        json to_json_impl(const py::handle& obj)
         {
             if (obj.is_none())
             {
@@ -155,7 +155,7 @@ namespace nlohmann
             if (py::isinstance<py::tuple>(obj) || py::isinstance<py::list>(obj))
             {
                 json out;
-                for (py::handle value: obj)
+                for (const py::handle& value: obj)
                 {
                     out.push_back(to_json_impl(value));
                 }
@@ -164,7 +164,7 @@ namespace nlohmann
             if (py::isinstance<py::dict>(obj))
             {
                 json out;
-                for (py::handle key: obj)
+                for (const py::handle& key: obj)
                 {
                     out[key.cast<std::string>()] = to_json_impl(obj[key]);
                 }
@@ -179,7 +179,7 @@ namespace nlohmann
         return detail::from_json_impl(j);
     }
 
-    void adl_serializer<py::object>::to_json(json& j, py::object obj)
+    void adl_serializer<py::object>::to_json(json& j, const py::object& obj)
     {
         j = detail::to_json_impl(obj);
     }
diff --git a/src/xutils.hpp b/src/xutils.hpp
@@ -22,10 +22,16 @@
 namespace py = pybind11;
 namespace nl = nlohmann;
 
+#if PY_MAJOR_VERSION == 2
+    #define XPYT_BUILTINS "__builtin__"
+#else
+    #define XPYT_BUILTINS "builtins"
+#endif
+
 namespace xpyt
 {
     py::list zmq_buffers_to_pylist(const std::vector<zmq::message_t>& buffers);
-    std::vector<zmq::message_t> pylist_to_zmq_buffers(py::list bufferlist);
+    std::vector<zmq::message_t> pylist_to_zmq_buffers(const py::list& bufferlist);
 
     py::object cppmessage_to_pymessage(const xeus::xmessage& msg);
 }
@@ -36,7 +42,7 @@ namespace nlohmann
     struct adl_serializer<py::object>
     {
         static py::object from_json(const json& j);
-        static void to_json(json& j, py::object obj);
+        static void to_json(json& j, const py::object& obj);
     };
 }
 

Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,7 @@ namespace nl = nlohmann;`
`26`	`26`
`27`	`27`	`namespace xpyt`
`28`	`28`	`{`
`29`		`- xcomm::xcomm(py::args /args/, py::kwargs kwargs)`
	`29`	`+ xcomm::xcomm(const py::args& /args/, const py::kwargs& kwargs)`
`30`	`30`	`: m_comm(target(kwargs), id(kwargs))`
`31`	`31`	`{`
`32`	`32`	`m_comm.open(`
`@@ -55,7 +55,7 @@ namespace xpyt`
`55`	`55`	`return true;`
`56`	`56`	`}`
`57`	`57`
`58`		`- void xcomm::close(py::args /args/, py::kwargs kwargs)`
	`58`	`+ void xcomm::close(const py::args& /args/, const py::kwargs& kwargs)`
`59`	`59`	`{`
`60`	`60`	`m_comm.close(`
`61`	`61`	`kwargs.attr("get")("metadata", py::dict()),`
`@@ -64,7 +64,7 @@ namespace xpyt`
`64`	`64`	`);`
`65`	`65`	`}`
`66`	`66`
`67`		`- void xcomm::send(py::args /args/, py::kwargs kwargs)`
	`67`	`+ void xcomm::send(const py::args& /args/, const py::kwargs& kwargs)`
`68`	`68`	`{`
`69`	`69`	`m_comm.send(`
`70`	`70`	`kwargs.attr("get")("metadata", py::dict()),`
`@@ -73,23 +73,23 @@ namespace xpyt`
`73`	`73`	`);`
`74`	`74`	`}`
`75`	`75`
`76`		`- void xcomm::on_msg(python_callback_type callback)`
	`76`	`+ void xcomm::on_msg(const python_callback_type& callback)`
`77`	`77`	`{`
`78`	`78`	`m_comm.on_message(cpp_callback(callback));`
`79`	`79`	`}`
`80`	`80`
`81`		`- void xcomm::on_close(python_callback_type callback)`
	`81`	`+ void xcomm::on_close(const python_callback_type& callback)`
`82`	`82`	`{`
`83`	`83`	`m_comm.on_close(cpp_callback(callback));`
`84`	`84`	`}`
`85`	`85`
`86`		`- xeus::xtarget* xcomm::target(py::kwargs kwargs) const`
	`86`	`+ xeus::xtarget* xcomm::target(const py::kwargs& kwargs) const`
`87`	`87`	`{`
`88`	`88`	`std::string target_name = kwargs["target_name"].cast<std::string>();`
`89`	`89`	`return xeus::get_interpreter().comm_manager().target(target_name);`
`90`	`90`	`}`
`91`	`91`
`92`		`- xeus::xguid xcomm::id(py::kwargs kwargs) const`
	`92`	`+ xeus::xguid xcomm::id(const py::kwargs& kwargs) const`
`93`	`93`	`{`
`94`	`94`	`if (py::hasattr(kwargs, "comm_id"))`
`95`	`95`	`{`
`@@ -102,7 +102,7 @@ namespace xpyt`
`102`	`102`	`}`
`103`	`103`	`}`
`104`	`104`
`105`		`- auto xcomm::cpp_callback(python_callback_type py_callback) const -> cpp_callback_type`
	`105`	`+ auto xcomm::cpp_callback(const python_callback_type& py_callback) const -> cpp_callback_type`
`106`	`106`	`{`
`107`	`107`	`return [this, py_callback](const xeus::xmessage& msg) {`
`108`	`108`	`py_callback(cppmessage_to_pymessage(msg));`
`@@ -117,9 +117,9 @@ namespace xpyt`
`117`	`117`	`{`
`118`	`118`	`}`
`119`	`119`
`120`		`- void register_target(py::str target_name, py::object callback)`
	`120`	`+ void register_target(const py::str& target_name, const py::object& callback)`
`121`	`121`	`{`
`122`		`- auto target_callback = [target_name, callback] (xeus::xcomm&& comm, const xeus::xmessage& msg) {`
	`122`	`+ auto target_callback = [&callback] (xeus::xcomm&& comm, const xeus::xmessage& msg) {`
`123`	`123`	`callback(xcomm(std::move(comm)), cppmessage_to_pymessage(msg));`
`124`	`124`	`};`
`125`	`125`
Original file line number	Diff line number	Diff line change
`@@ -37,7 +37,7 @@ namespace xpyt`
`37`	`37`	`m_execution_count = execution_count;`
`38`	`38`	`}`
`39`	`39`
`40`		`- void xdisplayhook::operator()(py::object obj, bool raw = false)`
	`40`	`+ void xdisplayhook::operator()(const py::object& obj, bool raw = false) const`
`41`	`41`	`{`
`42`	`42`	`auto& interp = xeus::get_interpreter();`
`43`	`43`
`@@ -67,7 +67,7 @@ namespace xpyt`
`67`	`67`	`}`
`68`	`68`	`}`
`69`	`69`
`70`		`- nl::json display_pub_data(py::object obj)`
	`70`	`+ nl::json xdisplayhook::display_pub_data(const py::object& obj) const`
`71`	`71`	`{`
`72`	`72`	`py::module py_json = py::module::import("json");`
`73`	`73`	`nl::json pub_data;`
Original file line number	Diff line number	Diff line change
`@@ -48,7 +48,7 @@ namespace xpyt`
`48`	`48`	`return bufferlist;`
`49`	`49`	`}`
`50`	`50`
`51`		`- std::vector<zmq::message_t> pylist_to_zmq_buffers(py::list bufferlist)`
	`51`	`+ std::vector<zmq::message_t> pylist_to_zmq_buffers(const py::list& bufferlist)`
`52`	`52`	`{`
`53`	`53`	`std::vector<zmq::message_t> buffers;`
`54`	`54`	`for (py::handle buffer : bufferlist)`
`@@ -130,7 +130,7 @@ namespace nlohmann`
`130`	`130`	`}`
`131`	`131`	`}`
`132`	`132`
`133`		`- json to_json_impl(py::handle obj)`
	`133`	`+ json to_json_impl(const py::handle& obj)`
`134`	`134`	`{`
`135`	`135`	`if (obj.is_none())`
`136`	`136`	`{`
`@@ -155,7 +155,7 @@ namespace nlohmann`
`155`	`155`	`if (py::isinstance<py::tuple>(obj) \|\| py::isinstance<py::list>(obj))`
`156`	`156`	`{`
`157`	`157`	`json out;`
`158`		`- for (py::handle value: obj)`
	`158`	`+ for (const py::handle& value: obj)`
`159`	`159`	`{`
`160`	`160`	`out.push_back(to_json_impl(value));`
`161`	`161`	`}`
`@@ -164,7 +164,7 @@ namespace nlohmann`
`164`	`164`	`if (py::isinstance<py::dict>(obj))`
`165`	`165`	`{`
`166`	`166`	`json out;`
`167`		`- for (py::handle key: obj)`
	`167`	`+ for (const py::handle& key: obj)`
`168`	`168`	`{`
`169`	`169`	`out[key.cast<std::string>()] = to_json_impl(obj[key]);`
`170`	`170`	`}`
`@@ -179,7 +179,7 @@ namespace nlohmann`
`179`	`179`	`return detail::from_json_impl(j);`
`180`	`180`	`}`
`181`	`181`
`182`		`- void adl_serializer<py::object>::to_json(json& j, py::object obj)`
	`182`	`+ void adl_serializer<py::object>::to_json(json& j, const py::object& obj)`
`183`	`183`	`{`
`184`	`184`	`j = detail::to_json_impl(obj);`
`185`	`185`	`}`
Original file line number	Diff line number	Diff line change
`@@ -22,10 +22,16 @@`
`22`	`22`	`namespace py = pybind11;`
`23`	`23`	`namespace nl = nlohmann;`
`24`	`24`
	`25`	`+#if PY_MAJOR_VERSION == 2`
	`26`	`+ #define XPYT_BUILTINS "__builtin__"`
	`27`	`+#else`
	`28`	`+ #define XPYT_BUILTINS "builtins"`
	`29`	`+#endif`
	`30`	`+`
`25`	`31`	`namespace xpyt`
`26`	`32`	`{`
`27`	`33`	`py::list zmq_buffers_to_pylist(const std::vector<zmq::message_t>& buffers);`
`28`		`- std::vector<zmq::message_t> pylist_to_zmq_buffers(py::list bufferlist);`
	`34`	`+ std::vector<zmq::message_t> pylist_to_zmq_buffers(const py::list& bufferlist);`
`29`	`35`
`30`	`36`	`py::object cppmessage_to_pymessage(const xeus::xmessage& msg);`
`31`	`37`	`}`
`@@ -36,7 +42,7 @@ namespace nlohmann`
`36`	`42`	`struct adl_serializer<py::object>`
`37`	`43`	`{`
`38`	`44`	`static py::object from_json(const json& j);`
`39`		`- static void to_json(json& j, py::object obj);`
	`45`	`+ static void to_json(json& j, const py::object& obj);`
`40`	`46`	`};`
`41`	`47`	`}`
`42`	`48`