Address comments

Author: minggangw

lib/logging.js

@@ -173,7 +173,7 @@ class Logging {
       severity,
       message,
       caller.functionName,
-      parseInt(caller.lineNumber),
+      parseInt(caller.lineNumber, 10),
       caller.fileName
     );
   }

src/addon.cpp

@@ -13,14 +13,14 @@
 // limitations under the License.
 
 #include <node_api.h>
+#include <rcutils/logging.h>
 
 #include "macros.hpp"
 #include "rcl_action_bindings.hpp"
 #include "rcl_bindings.hpp"
 #include "rcl_handle.hpp"
 #include "rcl_lifecycle_bindings.hpp"
-#include "rcutils/logging.h"
-#include "rcutils/macros.h"
+#include "rcl_utilities.hpp"
 #include "shadow_node.hpp"
 
 bool IsRunningInElectronRenderer(const Napi::Env& env) {
@@ -46,6 +46,8 @@ Napi::Object InitModule(Napi::Env env, Napi::Object exports) {
     prog_name[end - prog_name] = 0;
   }
 #endif
+
+  // Init the C++ bindings.
   rclnodejs::StoreEnv(env);
   rclnodejs::InitBindings(env, exports);
   rclnodejs::InitAction(env, exports);

src/macros.hpp

@@ -19,17 +19,6 @@
 
 #include "rcutils/logging_macros.h"
 
-namespace rclnodejs {
-// Store a reference to the environment that can be used for error reporting
-inline Napi::Env& GetEnv() {
-  static thread_local Napi::Env env = nullptr;
-  return env;
-}
-
-// Call this at the beginning of each function that may throw errors
-inline void StoreEnv(Napi::Env current_env) { GetEnv() = current_env; }
-}  // namespace rclnodejs
-
 #define CHECK_OP_AND_THROW_ERROR_IF_NOT_TRUE(op, lhs, rhs, message) \
   {                                                                 \
     if (lhs op rhs) {                                               \

src/rcl_bindings.cpp

@@ -1502,15 +1502,8 @@ Napi::Value CreateArrayBufferFromAddress(const Napi::CallbackInfo& info) {
   memcpy(array_buffer.Data(), addr, length);
   free(addr);
 #else
-  // For nodejs > 12 or electron < 21, we will create a new `BackingStore` and
-  // take over the ownership of `addr`.
-  // std::unique_ptr<v8::BackingStore> backing =
-  // v8::ArrayBuffer::NewBackingStore(
-  //     addr, length,
-  //     [](void* data, size_t length, void* deleter_data) { free(data); },
-  //     nullptr);
-  // auto array_buffer =
-  //     Napi::ArrayBuffer::New(env, std::move(backing));
+  // For nodejs > 12 or electron < 21, we will take over the ownership of
+  // `addr`.
   auto array_buffer = Napi::ArrayBuffer::New(
       env, addr, length, [](Napi::Env /*env*/, void* data) { free(data); });
 #endif
@@ -1880,16 +1873,11 @@ Napi::Value PublishRawMessage(const Napi::CallbackInfo& info) {
   rcl_publisher_t* publisher = reinterpret_cast<rcl_publisher_t*>(
       RclHandle::Unwrap(info[0].As<Napi::Object>())->ptr());
 
-  // auto object = (info[1].As<v8::Object>()).ToLocalChecked();
   auto object = info[1].As<Napi::Buffer<char>>();
-  // v8::Local<v8::Object> object = napiBuffer;
   rcl_serialized_message_t serialized_msg =
       rmw_get_zero_initialized_serialized_message();
-  // serialized_msg.buffer_capacity = node::Buffer::Length(object);;
   serialized_msg.buffer_capacity = object.Length();
   serialized_msg.buffer_length = serialized_msg.buffer_capacity;
-  // serialized_msg.buffer =
-  // reinterpret_cast<uint8_t*>(node::Buffer::Data(object));
   serialized_msg.buffer = reinterpret_cast<uint8_t*>(object.Data());
 
   THROW_ERROR_IF_NOT_EQUAL(

src/rcl_handle.cpp

@@ -19,7 +19,7 @@
 
 #include <iostream>
 
-#include "macros.hpp"
+#include "rcl_utilities.hpp"
 
 namespace rclnodejs {
 
@@ -59,12 +59,8 @@ void RclHandle::SyncProperties() {
 }
 
 Napi::Value RclHandle::PropertiesGetter(const Napi::CallbackInfo& info) {
-  Napi::Env env = info.Env();
-
-  if (!properties_obj_.IsEmpty())
-    return properties_obj_.Value();
-  else
-    return env.Undefined();
+  return !properties_obj_.IsEmpty() ? properties_obj_.Value()
+                                    : info.Env().Undefined();
 }
 
 Napi::Value RclHandle::Release(const Napi::CallbackInfo& info) {

src/rcl_handle.hpp

@@ -31,6 +31,9 @@ class RclHandle : public Napi::ObjectWrap<RclHandle> {
                                   RclHandle* parent,
                                   std::function<void(void*)> deleter);
 
+  explicit RclHandle(const Napi::CallbackInfo& info);
+  ~RclHandle();
+
   void set_deleter(std::function<void(void*)> deleter) { deleter_ = deleter; }
 
   RclHandle* parent() { return parent_; }
@@ -42,23 +45,16 @@ class RclHandle : public Napi::ObjectWrap<RclHandle> {
   void Reset();
   void AddChild(RclHandle* child) { children_.insert(child); }
   void RemoveChild(RclHandle* child) { children_.erase(child); }
+  void SetBoolProperty(const std::string& name, bool value) {
+    properties_[name] = value;
+  }
   void SyncProperties();
 
-  explicit RclHandle(const Napi::CallbackInfo& info);
-  ~RclHandle();
-
- public:
-  // Methods
+ private:
   Napi::Value Release(const Napi::CallbackInfo& info);
   Napi::Value Dismiss(const Napi::CallbackInfo& info);
-
-  // Property getter
   Napi::Value PropertiesGetter(const Napi::CallbackInfo& info);
 
-  void SetBoolProperty(const std::string& name, bool value) {
-    properties_[name] = value;
-  }
-
  private:
   void* pointer_;
   RclHandle* parent_;

src/rcl_utilities.hpp

@@ -15,6 +15,8 @@
 #ifndef SRC_RCL_UTILITIES_HPP_
 #define SRC_RCL_UTILITIES_HPP_
 
+#include <napi.h>
+
 #include <string>
 
 struct rosidl_message_type_support_t;
@@ -35,6 +37,14 @@ const rosidl_action_type_support_t* GetActionTypeSupport(
 
 std::string GetErrorMessageAndClear();
 
+// Store a reference to the environment that can be used for error reporting.
+inline Napi::Env& GetEnv() {
+  static thread_local Napi::Env env = nullptr;
+  return env;
+}
+
+inline void StoreEnv(Napi::Env current_env) { GetEnv() = current_env; }
+
 }  // namespace rclnodejs
 
 #endif  // SRC_RCL_UTILITIES_HPP_