fix a bunch of Sonar smells

simgrid · May 24, 2024 · d37877a · d37877a
1 parent f5a4740
commit d37877a
Show file tree

Hide file tree

Showing 12 changed files with 28 additions and 42 deletions.
diff --git a/include/fsmod/FileSystem.hpp b/include/fsmod/FileSystem.hpp
@@ -35,7 +35,7 @@ namespace simgrid::fsmod {
 
         [[nodiscard]] bool file_exists(const std::string& full_path);
         [[nodiscard]] bool directory_exists(const std::string& full_dir_path);
-        std::set<std::string> list_files_in_directory(const std::string& full_dir_path);
+        std::set<std::string, std::less<>> list_files_in_directory(const std::string& full_dir_path);
 
         void move_file(const std::string& src_full_path, const std::string& dst_full_path) const;
         void unlink_file(const std::string& full_path) const;
@@ -55,7 +55,7 @@ namespace simgrid::fsmod {
     private:
         [[nodiscard]] std::pair<std::shared_ptr<Partition>, std::string> find_path_at_mount_point(const std::string &full_path) const;
 
-        std::map<std::string, std::shared_ptr<Partition>> partitions_;
+        std::map<std::string, std::shared_ptr<Partition>, std::less<>> partitions_;
 
         int num_open_files_ = 0;
     };

diff --git a/include/fsmod/FileSystemException.hpp b/include/fsmod/FileSystemException.hpp
@@ -3,20 +3,6 @@
 
 #include <simgrid/Exception.hpp>
 
-// Macro copied over from SimGrid source code. Is it a good idea?
-//#define DECLARE_SIMGRID_EXCEPTION(AnyException, ...)                                                                   \
-//  class AnyException : public simgrid::Exception {                                                                     \
-//  public:                                                                                                              \
-//    using simgrid::Exception::Exception;                                                                               \
-//    __VA_ARGS__                                                                                                        \
-//    ~AnyException() override;                                                                                          \
-//    XBT_ATTRIB_NORETURN void rethrow_nested(simgrid::xbt::ThrowPoint&& throwpoint,                                     \
-//                                            const std::string& message) const override                                 \
-//    {                                                                                                                  \
-//      std::throw_with_nested(AnyException(std::move(throwpoint), message));                                            \
-//    }                                                                                                                  \
-//  }
-
 namespace simgrid::fsmod {
 
     /**
@@ -25,9 +11,7 @@ namespace simgrid::fsmod {
     class FileSystemException : public std::exception {
         std::string msg_;
     public:
-        FileSystemException(simgrid::xbt::ThrowPoint&& throwpoint, const std::string &msg) {
-            msg_ = msg;
-        }
+        FileSystemException(simgrid::xbt::ThrowPoint&& throwpoint, std::string msg) : msg_(std::move(msg)){}
 
         /**
          * @brief Retrieves the exception's human-readable message

diff --git a/include/fsmod/JBODStorage.hpp b/include/fsmod/JBODStorage.hpp
@@ -57,14 +57,13 @@ namespace simgrid::fsmod {
 
         int get_next_read_disk_idx() { return (++read_disk_idx_) % num_disks_; }
         [[nodiscard]] int get_parity_disk_idx() const { return parity_disk_idx_; }
-        s4u::MessageQueue* mq_;
-
 
     private:
         unsigned int num_disks_;
         RAID raid_level_;
         unsigned int parity_disk_idx_;
         int read_disk_idx_ = -1;
+        s4u::MessageQueue* mq_;
     };
 }
 

diff --git a/include/fsmod/OneDiskStorage.hpp b/include/fsmod/OneDiskStorage.hpp
@@ -20,6 +20,8 @@ namespace simgrid::fsmod {
 
     protected:
         OneDiskStorage(const std::string &name, simgrid::s4u::Disk *disk);
+
+    private:
         s4u::MessageQueue *mq_;
     };
 } // namespace simgrid::fsmod

diff --git a/include/fsmod/Partition.hpp b/include/fsmod/Partition.hpp
@@ -85,7 +85,7 @@ namespace simgrid::fsmod {
         [[nodiscard]] std::shared_ptr<Storage> get_storage() const { return storage_; }
 
         [[nodiscard]] bool directory_exists(const std::string& dir_path) const { return content_.find(dir_path) != content_.end(); }
-        std::set<std::string> list_files_in_directory(const std::string &dir_path) const;
+        std::set<std::string, std::less<>> list_files_in_directory(const std::string &dir_path) const;
         void delete_directory(const std::string &dir_path);
 
         [[nodiscard]] FileMetadata* get_file_metadata(const std::string& dir_path, const std::string& file_name) const;

diff --git a/src/FileSystem.cpp b/src/FileSystem.cpp
@@ -25,7 +25,7 @@ namespace simgrid::fsmod {
         // Identify the mount point and path at mount point partition
         auto it = std::find_if(this->partitions_.begin(),
                                this->partitions_.end(),
-                               [simplified_path](const std::pair<std::string, std::shared_ptr<Partition>> &element) {
+                               [&simplified_path](const std::pair<std::string, std::shared_ptr<Partition>> &element) {
                                    return PathUtil::is_at_mount_point(simplified_path, element.first);
                                });
         if (it == this->partitions_.end()) {
@@ -39,7 +39,7 @@ namespace simgrid::fsmod {
     /*********************** PUBLIC INTERFACE *****************************/
 
     std::shared_ptr<FileSystem> FileSystem::create(const std::string &name, int max_num_open_files) {
-        return std::shared_ptr<FileSystem>(new FileSystem(name, max_num_open_files));
+        return std::make_shared<FileSystem>(FileSystem(name, max_num_open_files));
     }
 
     /**
@@ -161,7 +161,7 @@ namespace simgrid::fsmod {
         if (not metadata) {
             if (access_mode == "r")
                 throw FileSystemException(XBT_THROW_POINT, "File not found. Cannot be opened in 'r' mode");
-            create_file(full_path, 0);
+            create_file(full_path, "0B");
             metadata = partition->get_file_metadata(dir, file_name);
         } else {
             if (access_mode == "w") {
@@ -279,7 +279,7 @@ namespace simgrid::fsmod {
      * @param full_dir_path: the path to the directory
      * @return
      */
-    std::set<std::string> FileSystem::list_files_in_directory(const std::string &full_dir_path) {
+    std::set<std::string, std::less<>> FileSystem::list_files_in_directory(const std::string &full_dir_path) {
         std::string simplified_path = PathUtil::simplify_path_string(full_dir_path);
         auto [partition, path_at_mount_point] = this->find_path_at_mount_point(simplified_path);
         return partition->list_files_in_directory(path_at_mount_point);

diff --git a/src/JBODStorage.cpp b/src/JBODStorage.cpp
@@ -22,15 +22,16 @@ namespace simgrid::fsmod {
         return storage;
     }
 
-    JBODStorage::JBODStorage(const std::string& name, const std::vector<s4u::Disk*>& disks) : Storage(name) {
+    JBODStorage::JBODStorage(const std::string& name, const std::vector<s4u::Disk*>& disks)
+        : Storage(name),
+          num_disks_(disks.size()) {
         disks_ = disks;
-        num_disks_ = disks.size();
         parity_disk_idx_ = num_disks_ - 1;
         controller_host_ = disks_.front()->get_host();
         // Create a no-op controller
         mq_ = s4u::MessageQueue::by_name(name+"_controller_mq");
         controller_ = s4u::Actor::create(name+"_controller", controller_host_, [this](){
-            //mq_->get<void*>();
+            // Do nothing
         });
         controller_->daemonize();
     }
@@ -90,7 +91,7 @@ namespace simgrid::fsmod {
                 XBT_DEBUG("%s", debug_msg.str().c_str());
                 break;
             default:
-                throw std::runtime_error("Unsupported RAID level. Supported level are: 0, 1, 4, 5, and 6");
+                throw std::invalid_argument("Unsupported RAID level. Supported level are: 0, 1, 4, 5, and 6");
         }
 
         // Create a Comm to transfer data to the host that requested a read to the controller host of the JBOD
@@ -155,7 +156,7 @@ namespace simgrid::fsmod {
                 write_size = size / (num_disks_ - 2);
                 break;
             default:
-                throw std::runtime_error("Unsupported RAID level. Supported level are: 0, 1, 4, 5, and 6");
+                throw std::invalid_argument("Unsupported RAID level. Supported level are: 0, 1, 4, 5, and 6");
         }
 
         // Compute the parity block (if any)

diff --git a/src/Partition.cpp b/src/Partition.cpp
@@ -121,11 +121,11 @@ namespace simgrid::fsmod {
         content_[dst_dir_path][dst_file_name] = std::move(uniq_ptr);
     }
 
-    std::set<std::string> Partition::list_files_in_directory(const std::string &dir_path) const {
+    std::set<std::string, std::less<>> Partition::list_files_in_directory(const std::string &dir_path) const {
         if (content_.find(dir_path) == content_.end()) {
             throw FileSystemException(XBT_THROW_POINT, "Directory does not exist");
         }
-        std::set < std::string > keys;
+        std::set<std::string, std::less<>> keys;
         for (auto const &key: content_.at(dir_path)) {
             keys.insert(key.first);
         }

diff --git a/test/file_system_test.cpp b/test/file_system_test.cpp
@@ -114,7 +114,7 @@ TEST_F(FileSystemTest, Directories)  {
             ASSERT_DOUBLE_EQ(fs_->partition_by_name("/dev/a//////")->get_free_space(), 80*1000);
             XBT_INFO("Create a 10kB file at /dev/a/b/c/faa.txt");
             ASSERT_NO_THROW(fs_->create_file("/dev/a/b/c/faa.txt", "10kB"));
-            std::set<std::string> found_files;
+            std::set<std::string, std::less<>> found_files;
             ASSERT_THROW(found_files = fs_->list_files_in_directory("/dev/a/b/c_bogus"), sgfs::FileSystemException);
             ASSERT_NO_THROW(found_files = fs_->list_files_in_directory("/dev/a/b/c"));
             ASSERT_TRUE(found_files.find("foo.txt") != found_files.end());

diff --git a/test/jbod_storage_test.cpp b/test/jbod_storage_test.cpp
@@ -210,13 +210,13 @@ TEST_F(JBODStorageTest, ReadWriteUnsupportedRAID)  {
             XBT_INFO("Open File '/dev/a/foo.txt' in write mode");
             ASSERT_NO_THROW(file = fs_->open("/dev/a/foo.txt", "w"));
             XBT_INFO("Write 2MB at /dev/a/foo.txt, which should fail");
-            ASSERT_THROW(file->write("12MB"), std::runtime_error);
+            ASSERT_THROW(file->write("12MB"), std::invalid_argument);
             XBT_INFO("Close the file");
             ASSERT_NO_THROW(fs_->close(file));
             XBT_INFO("Open File '/dev/a/foo.txt' in read mode");
             ASSERT_NO_THROW(file = fs_->open("/dev/a/foo.txt", "r"));
             XBT_INFO("Read 12MB at /dev/a/foo.txt, which should fail too");
-            ASSERT_THROW(file->read("12MB"), std::runtime_error);
+            ASSERT_THROW(file->read("12MB"), std::invalid_argument);
             XBT_INFO("Close the file");
             ASSERT_NO_THROW(fs_->close(file));
         });

diff --git a/test/path_util_test.cpp b/test/path_util_test.cpp
@@ -39,11 +39,11 @@ TEST_F(PathUtilTest, SplitPath) {
             {"/",                            {"/", ""}},
             {"a",                            {"/", "a"}},
     };
-    for (const auto &test_item: input_output) {
-        auto simplified_path = sgfs::PathUtil::simplify_path_string(test_item.first);
+    for (const auto& [input, output]: input_output) {
+        auto simplified_path = sgfs::PathUtil::simplify_path_string(input);
         auto [dir, file] = sgfs::PathUtil::split_path(simplified_path);
-        MY_ASSERT_EQ(dir, test_item.second.first, test_item.first + " (wrong directory)");
-        MY_ASSERT_EQ(file, test_item.second.second, test_item.first + " (wrong file)");
+        MY_ASSERT_EQ(dir, output.first, input + " (wrong directory)");
+        MY_ASSERT_EQ(file, output.second, input + " (wrong file)");
     }
     // test this case without simplifuing the path first
     std::string path = "a";
@@ -66,7 +66,7 @@ TEST_F(PathUtilTest, PathAtMountPoint) {
     };
 
     for (const auto &test_item: input_output) {
-        auto [path, mp, output] = test_item;
+        const auto [path, mp, output] = test_item;
         auto simplified_path = sgfs::PathUtil::simplify_path_string(path);
         try {
             auto path_at_mp = sgfs::PathUtil::path_at_mount_point(simplified_path, "/dev/a");

diff --git a/test/test_util.hpp b/test/test_util.hpp
@@ -10,7 +10,7 @@ static void DO_TEST_WITH_FORK(const std::function<void()> &lambda) {
         ASSERT_EQ(exit_code, 0);
     } else {
         lambda();
-        exit((::testing::Test::HasFailure() ? 255 : 0));
+        exit(::testing::Test::HasFailure() ? 255 : 0);
     }
 }