Claude code review

Author: frs69wq

CMakeLists.txt

@@ -15,16 +15,16 @@ find_package(SimGrid 4.1 REQUIRED)
 find_package(FSMod 0.3.1 REQUIRED)
 find_package(nlohmann_json REQUIRED)
 
-include_directories(
-  ${CMAKE_SOURCE_DIR}/include
-	${SimGrid_INCLUDE_DIR}
-  ${FSMod_INCLUDE_DIR}
-  ${CMAKE_BINARY_DIR}/include
-)
+# Note: Global include_directories removed in favor of target-specific includes below
 
 set(CMAKE_CXX_STANDARD 17)
 set(CMAKE_CXX_STANDARD_REQUIRED ON)
-set(CMAKE_CXX_FLAGS "-O3 -funroll-loops -fno-strict-aliasing -flto=auto")
+set(CMAKE_CXX_FLAGS_RELEASE "-O3 -funroll-loops -fno-strict-aliasing -flto=auto -DNDEBUG")
+set(CMAKE_CXX_FLAGS_DEBUG "-O0 -g3 -Wall -Wextra")
+
+if(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang")
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Wpedantic")
+endif()
 
 # build the version number
 set(DTLMOD_VERSION_MAJOR "0")
@@ -109,7 +109,7 @@ if(enable_python)
                         ${python_files}
                         ${pybind11_options})
 
-    target_compile_features(python-bindings PRIVATE cxx_std_14)
+    target_compile_features(python-bindings PRIVATE cxx_std_17)
     target_link_libraries(python-bindings PUBLIC dtlmod)
     set_target_properties(python-bindings PROPERTIES
                           LIBRARY_OUTPUT_NAME dtlmod
@@ -178,10 +178,18 @@ set_target_properties(dtlmod PROPERTIES
   LINKER_LANGUAGE CXX
   PUBLIC_HEADER "${HEADER_FILES}")
 
-target_include_directories(dtlmod PRIVATE include)
+target_include_directories(dtlmod
+  PUBLIC
+    $<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/include>
+    $<BUILD_INTERFACE:${CMAKE_BINARY_DIR}/include>
+    $<INSTALL_INTERFACE:include>
+  PRIVATE
+    ${SimGrid_INCLUDE_DIR}
+    ${FSMod_INCLUDE_DIR}
+)
 target_link_libraries(dtlmod PUBLIC ${SimGrid_LIBRARY} ${FSMOD_LIBRARY})
 
-foreach (conf "${CONFIG_FILES}")
+foreach (conf ${CONFIG_FILES})
   add_custom_command(
     TARGET ${PROJECT_NAME} POST_BUILD
     COMMAND ${CMAKE_COMMAND} -E copy
@@ -210,8 +218,13 @@ if(GTEST_LIBRARY)
 
   add_definitions(-DGTEST_USED)
   add_executable(unit_tests EXCLUDE_FROM_ALL ${SOURCE_FILES} ${HEADER_FILES} ${TEST_FILES})
-  target_include_directories(unit_tests PRIVATE include)
-  target_link_libraries(unit_tests ${GTEST_LIBRARY} ${SIMGRID_LIBRARY} ${FSMOD_LIBRARY} dtlmod -lpthread -lm)
+  target_include_directories(unit_tests PRIVATE
+    ${CMAKE_SOURCE_DIR}/include
+    ${CMAKE_BINARY_DIR}/include
+    ${SimGrid_INCLUDE_DIR}
+    ${FSMod_INCLUDE_DIR}
+  )
+  target_link_libraries(unit_tests ${GTEST_LIBRARY} ${SimGrid_LIBRARY} ${FSMOD_LIBRARY} dtlmod -lpthread -lm)
   set_target_properties(unit_tests PROPERTIES COMPILE_FLAGS "-g -O0 --coverage")
   set_target_properties(unit_tests PROPERTIES LINK_FLAGS "--coverage")
   #add_custom_command(TARGET unit_tests COMMAND find . -name *.gcda -delete)

include/dtlmod/DTL.hpp

@@ -6,6 +6,8 @@
 #ifndef __DTLMOD_DTL_HPP__
 #define __DTLMOD_DTL_HPP__
 
+#include <optional>
+
 #include <xbt/log.h>
 
 #include "dtlmod/Stream.hpp"
@@ -35,15 +37,13 @@ class DTL {
   explicit DTL(const std::string& filename);
   /// \endcond
 
-  /// @brief Create the Data Transport Layer.
-  static void create();
   /// @brief Create the Data Transport Layer.
   /// @param filename: a JSON configuration file that provide stream parameters.
-  static void create(const std::string& filename);
+  static void create(const std::string& filename = "");
 
   /// @brief Connect an Actor to the Data Transport Layer.
   /// @return A handler on the DTL object.
-  static std::shared_ptr<DTL> connect();
+  [[nodiscard]] static std::shared_ptr<DTL> connect();
   /// @brief Disconnect an Actor from the Data Transport Layer.
   static void disconnect();
 
@@ -54,16 +54,16 @@ class DTL {
   /// @brief Add a data stream to the Data Transport Layer.
   /// @param name The name of the Stream to add to the DTL.
   /// @return A handler on the newly created Stream object.
-  std::shared_ptr<Stream> add_stream(const std::string& name);
+  [[nodiscard]] std::shared_ptr<Stream> add_stream(const std::string& name);
 
   /// @brief Retrieve all streams declared in the Data Transport Layer.
   /// @return a map of handlers on Stream objects with their names as keys.
   const std::unordered_map<std::string, std::shared_ptr<Stream>>& get_all_streams() const { return streams_; }
 
   /// @brief Retrieve a data stream from the Data Transport Layer by its name.
   /// @param name The name of the Stream to retrieve.
-  /// @return A handler on the Stream object or nullptr if it doesn't exist.
-  std::shared_ptr<Stream> get_stream_by_name_or_null(const std::string& name) const;
+  /// @return An optional containing the Stream handler if found, std::nullopt otherwise.
+  [[nodiscard]] std::optional<std::shared_ptr<Stream>> get_stream_by_name(const std::string& name) const;
 };
 
 } // namespace dtlmod

include/dtlmod/Engine.hpp

@@ -34,7 +34,7 @@ class Engine {
 public:
   /// @brief An enum that defines the type of Engine supported by the DTL
   enum class Type {
-    /// @brief The Engine Type has not nee specified yet.
+    /// @brief The Engine Type has not been specified yet.
     Undefined,
     /// @brief File Engine. Relies on files written to and read from a file system to transport data from publisher(s)
     /// to subscriber(s).

include/dtlmod/Metadata.hpp

@@ -32,16 +32,19 @@ class Metadata {
 protected:
   const std::map<std::pair<std::vector<size_t>, std::vector<size_t>>, std::pair<std::string, sg4::ActorPtr>,
                  std::less<>>&
-  get_blocks_for_transaction(int id)
+  get_blocks_for_transaction(unsigned int id)
   {
     return transaction_infos_[id];
   }
-  void add_transaction(int id, const std::pair<std::vector<size_t>, std::vector<size_t>>& start_and_count,
+  void add_transaction(unsigned int id, const std::pair<std::vector<size_t>, std::vector<size_t>>& start_and_count,
                        const std::string& filename, sg4::ActorPtr publisher);
 
 public:
   explicit Metadata(Variable* variable) : variable_(variable) {}
-  int get_current_transaction() const { return transaction_infos_.empty() ? -1 : (transaction_infos_.rbegin())->first; }
+  unsigned int get_current_transaction() const
+  {
+    return transaction_infos_.empty() ? 0 : (transaction_infos_.rbegin())->first;
+  }
   void export_to_file(std::ofstream& ostream) const;
 };
 /// \endcond

include/dtlmod/StagingEngine.hpp

@@ -6,6 +6,8 @@
 #ifndef __DTLMOD_ENGINE_STAGING_HPP__
 #define __DTLMOD_ENGINE_STAGING_HPP__
 
+#include <atomic>
+
 #include "dtlmod/Engine.hpp"
 
 XBT_LOG_EXTERNAL_CATEGORY(dtlmod);
@@ -16,7 +18,7 @@ namespace dtlmod {
 class StagingEngine : public Engine {
   sg4::ConditionVariablePtr first_pub_transaction_started_ = sg4::ConditionVariable::create();
   sg4::ConditionVariablePtr sub_transaction_started_       = sg4::ConditionVariable::create();
-  int num_subscribers_starting_                            = 0;
+  std::atomic<unsigned int> num_subscribers_starting_{0};
   bool pub_closing_                                        = false;
   bool sub_closing_                                        = false;
 

include/dtlmod/Stream.hpp

@@ -46,6 +46,7 @@ class Stream : public std::enable_shared_from_this<Stream> {
   Transport::Method transport_method_ = Transport::Method::Undefined;
   bool metadata_export_               = false;
   sg4::MutexPtr mutex_                = sg4::Mutex::create();
+  sg4::ConditionVariablePtr engine_created_ = sg4::ConditionVariable::create();
   Mode access_mode_;
 
   std::unordered_map<std::string, std::shared_ptr<Variable>> variables_;
@@ -59,7 +60,7 @@ class Stream : public std::enable_shared_from_this<Stream> {
   }
   [[nodiscard]] const char* mode_to_str(Mode mode) const
   {
-    return (mode == Mode::Publish) ? "Mode::Publish" : "Mode::Suscribe";
+    return (mode == Mode::Publish) ? "Mode::Publish" : "Mode::Subscribe";
   }
   void close() { engine_ = nullptr; }
   /// \endcond
@@ -115,7 +116,7 @@ class Stream : public std::enable_shared_from_this<Stream> {
   /// @param name name of the Engine created when opening the Stream.
   /// @param mode either Stream::Mode::Publish or Stream::Mode::Subscribe.
   /// @return A shared pointer on the corresponding Engine.
-  std::shared_ptr<Engine> open(const std::string& name, Mode mode);
+  [[nodiscard]] std::shared_ptr<Engine> open(const std::string& name, Mode mode);
 
   /// @brief Helper function to obtain the number of actors connected to Stream in Mode::Publish.
   /// @return The number of publishers for that Stream.
@@ -130,7 +131,7 @@ class Stream : public std::enable_shared_from_this<Stream> {
   /// @param name The name of the new Variable.
   /// @param element_size The size of the elements in the Variable.
   /// @return A shared pointer on the newly created Variable.
-  std::shared_ptr<Variable> define_variable(const std::string& name, size_t element_size);
+  [[nodiscard]] std::shared_ptr<Variable> define_variable(const std::string& name, size_t element_size);
 
   /// @brief Define a Variable for this Stream.
   /// @param name The name of the new variable.
@@ -139,18 +140,18 @@ class Stream : public std::enable_shared_from_this<Stream> {
   /// @param count A vector that specifies how many elements the calling Actor owns in each dimension.
   /// @param element_size The size of the elements in the Variable.
   /// @return A shared pointer on the newly created Variable
-  std::shared_ptr<Variable> define_variable(const std::string& name, const std::vector<size_t>& shape,
-                                            const std::vector<size_t>& start, const std::vector<size_t>& count,
-                                            size_t element_size);
+  [[nodiscard]] std::shared_ptr<Variable> define_variable(const std::string& name, const std::vector<size_t>& shape,
+                                                          const std::vector<size_t>& start,
+                                                          const std::vector<size_t>& count, size_t element_size);
 
   /// @brief Retrieve the list of Variables defined on this stream
   /// @return the list of Variable names
-  std::vector<std::string> get_all_variables() const;
+  [[nodiscard]] std::vector<std::string> get_all_variables() const;
 
   /// @brief Retrieve a Variable information by name.
   /// @param name The name of desired Variable.
   /// @return Either a shared pointer on the Variable object if known, nullptr otherwise.
-  std::shared_ptr<Variable> inquire_variable(const std::string& name) const;
+  [[nodiscard]] std::shared_ptr<Variable> inquire_variable(const std::string& name) const;
 
   /// @brief Remove a Variable of the list of variables known by the Stream.
   /// @param name The name of the variable to remove.

include/dtlmod/Variable.hpp

@@ -28,7 +28,7 @@ class Variable {
   size_t element_size_;
   std::vector<size_t> shape_;
   std::unordered_map<sg4::ActorPtr, std::pair<std::vector<size_t>, std::vector<size_t>>> local_start_and_count_;
-  int transaction_start_          = -1;
+  unsigned int transaction_start_ = 0;
   unsigned int transaction_count_ = 0;
 
   std::weak_ptr<const Stream> defined_in_stream_;
@@ -41,19 +41,19 @@ class Variable {
 protected:
   /// \cond EXCLUDE_FROM_DOCUMENTATION
   void set_metadata(std::shared_ptr<Metadata> metadata) { metadata_ = metadata; }
-  void set_transaction_start(int start) { transaction_start_ = start; }
-  [[nodiscard]] int get_transaction_start() const { return transaction_start_; }
-  void set_transaction_count(int count) { transaction_count_ = count; }
+  void set_transaction_start(unsigned int start) { transaction_start_ = start; }
+  [[nodiscard]] unsigned int get_transaction_start() const { return transaction_start_; }
+  void set_transaction_count(unsigned int count) { transaction_count_ = count; }
   [[nodiscard]] unsigned int get_transaction_count() const { return transaction_count_; }
 
   void set_local_start_and_count(sg4::ActorPtr actor,
                                  const std::pair<std::vector<size_t>, std::vector<size_t>>& local_start_and_count)
   {
     local_start_and_count_[actor] = local_start_and_count;
   }
-  const std::pair<std::vector<size_t>, std::vector<size_t>>& get_local_start_and_count(sg4::ActorPtr actor)
+  const std::pair<std::vector<size_t>, std::vector<size_t>>& get_local_start_and_count(sg4::ActorPtr actor) const
   {
-    return local_start_and_count_[actor];
+    return local_start_and_count_.at(actor);
   }
 
   void add_transaction_metadata(unsigned int transaction_id, sg4::ActorPtr publisher, const std::string& location);

src/DTL.cpp

@@ -18,9 +18,8 @@ namespace sg4 = simgrid::s4u;
 namespace dtlmod {
 
 /// \cond EXCLUDE_FROM_DOCUMENTATION
-DTL::DTL(const std::string& filename)
+DTL::DTL(const std::string& filename) : mutex_(sg4::Mutex::create())
 {
-  mutex_ = sg4::Mutex::create();
   // No configuration file has been provided. Nothing else to do
   if (filename.empty())
     return;
@@ -96,8 +95,9 @@ __attribute__((noreturn)) void DTL::connection_manager_init(std::shared_ptr<DTL>
     bool* connect = nullptr;
     auto mess     = connect_mq->get_async(&connect);
     mess->wait();
+    std::unique_ptr<bool> connect_guard(connect); // RAII: automatic cleanup
     auto* sender = mess->get_sender();
-    if (*connect) { // Connection
+    if (*connect_guard) { // Connection
       dtl->connection_manager_connect(sender);
       // Return a handler on the DTL to the newly connected actor
       handler_mq->put_init(&dtl)->detach();
@@ -107,7 +107,7 @@ __attribute__((noreturn)) void DTL::connection_manager_init(std::shared_ptr<DTL>
       if (!dtl->has_active_connections())
         XBT_WARN("The DTL has no active connection");
     }
-    delete connect;
+    // connect_guard automatically deletes connect at end of scope
   } while (true);
 }
 ///\endcond
@@ -124,11 +124,6 @@ void DTL::create(const std::string& filename)
       ->daemonize();
 }
 
-void DTL::create()
-{
-  create("");
-}
-
 std::shared_ptr<DTL> DTL::connect()
 {
   sg4::MessageQueue::by_name("dtlmod::connection_manager_connect")->put(new bool(true));
@@ -152,11 +147,11 @@ std::shared_ptr<Stream> DTL::add_stream(const std::string& name)
   return streams_[name];
 }
 
-std::shared_ptr<Stream> DTL::get_stream_by_name_or_null(const std::string& name) const
+std::optional<std::shared_ptr<Stream>> DTL::get_stream_by_name(const std::string& name) const
 {
   auto it = streams_.find(name);
   if (it == streams_.end())
-    return nullptr;
+    return std::nullopt;
   return it->second;
 }
 } // namespace dtlmod

src/FileEngine.cpp

@@ -65,7 +65,7 @@ FileEngine::FileEngine(const std::string& fullpath, const std::shared_ptr<Stream
   }
 }
 
-void FileEngine::create_transport(const Transport::Method& transport_method)
+void FileEngine::create_transport(const Transport::Method& /*transport_method*/)
 {
   // This function is called in a critical section. Transport can only be created once.
   set_transport(std::make_shared<FileTransport>(this));

src/Metadata.cpp

@@ -11,7 +11,8 @@ XBT_LOG_NEW_DEFAULT_SUBCATEGORY(dtlmod_metadata, dtlmod, "DTL logging about Meta
 
 namespace dtlmod {
 /// \cond EXCLUDE_FROM_DOCUMENTATION
-void Metadata::add_transaction(int id, const std::pair<std::vector<size_t>, std::vector<size_t>>& start_and_count,
+void Metadata::add_transaction(unsigned int id,
+                               const std::pair<std::vector<size_t>, std::vector<size_t>>& start_and_count,
                                const std::string& location, sg4::ActorPtr publisher)
 {
   transaction_infos_[id][start_and_count] = std::make_pair(location, publisher);