more smells

include/fsmod/File.hpp

@@ -29,20 +29,16 @@ namespace simgrid::fsmod {
         int write_init_checks(sg_size_t num_bytes);
 
     public:
-        virtual ~File() = default;
-        File(const File&) = delete;
-
-    protected:
         File(std::string full_path, std::string access_mode, FileMetadata *metadata,
              Partition *partition)
             : path_(std::move(full_path)),
               access_mode_(std::move(access_mode)),
               metadata_(metadata),
               partition_(partition) {};
-
+        File(const File&) = delete;
         File& operator=(const File&) = delete;
+        virtual ~File() = default;
 
-    public:
         /** Get the number of bytes actually read by a given I/O Read activity */
         sg_size_t get_num_bytes_read(const s4u::IoPtr& read) const { return read->get_performed_ioops(); }
         /** Get the number of bytes actually written by a given I/O Write activity */

src/File.cpp

@@ -91,7 +91,6 @@ namespace simgrid::fsmod {
 
         if (added_bytes > partition_->get_free_space()) {
             partition_->create_space(added_bytes - partition_->get_free_space());
-//            throw FileSystemException(XBT_THROW_POINT, "Not enough space");
         }
 
         // Compute the new tentative file size

src/FileSystem.cpp

@@ -68,8 +68,8 @@ namespace simgrid::fsmod {
         if (PathUtil::simplify_path_string(mount_point) != cleanup_mount_point) {
             throw std::invalid_argument("Invalid partition path");
         }
-        for (auto const &mp: this->partitions_) {
-            if ((mp.first.rfind(cleanup_mount_point, 0) == 0) || (cleanup_mount_point.rfind(mp.first, 0) == 0)) {
+        for (auto const &[mp, p]: this->partitions_) {
+            if ((mp.rfind(cleanup_mount_point, 0) == 0) || (cleanup_mount_point.rfind(mp, 0) == 0)) {
                 throw std::invalid_argument("Mount point already exists or is prefix of existing mount point");
             }
         }
@@ -172,7 +172,7 @@ namespace simgrid::fsmod {
         metadata->increase_file_refcount();
 
         // Create the file object
-        auto file = std::shared_ptr<File>(new File(simplified_path, access_mode, metadata, partition.get()));
+        auto file = std::make_shared<File>(simplified_path, access_mode, metadata, partition.get());
 
         XBT_INFO("%s %d", access_mode.c_str(), SEEK_END);
         if (access_mode == "a")

src/Partition.cpp

@@ -21,7 +21,7 @@ namespace simgrid::fsmod {
     FileMetadata *Partition::get_file_metadata(const std::string &dir_path, const std::string &file_name) const {
         try {
             return content_.at(dir_path).at(file_name).get();
-        } catch (std::out_of_range &e) {
+        } catch (std::out_of_range&) {
             return nullptr;
         }
     }
@@ -126,8 +126,8 @@ namespace simgrid::fsmod {
             throw FileSystemException(XBT_THROW_POINT, "Directory does not exist");
         }
         std::set<std::string, std::less<>> keys;
-        for (auto const &key: content_.at(dir_path)) {
-            keys.insert(key.first);
+        for (auto const &[filename, metadata]: content_.at(dir_path)) {
+            keys.insert(filename);
         }
         return keys;
     }
@@ -137,8 +137,8 @@ namespace simgrid::fsmod {
             throw FileSystemException(XBT_THROW_POINT, "Directory does not exist");
         }
         // Check that no file is open
-        for (const auto &item: content_.at(dir_path)) {
-            if (item.second->get_file_refcount() != 0) {
+        for (const auto &[filename, metadata]: content_.at(dir_path)) {
+            if (metadata->get_file_refcount() != 0) {
                 throw FileSystemException(XBT_THROW_POINT,
                                           "Cannot delete a file that is open - no content deleted in directory");
             }

test/file_system_test.cpp

@@ -77,7 +77,7 @@ TEST_F(FileSystemTest, FileCreate)  {
             try {
                 this->fs_->create_file("/foo/foo.txt", "10MB");
             } catch (sgfs::FileSystemException &e) {
-                auto msg = e.what();  // coverage
+                XBT_ERROR("%s", e.what());  // coverage
             }
             XBT_INFO("Create a 10MB file at /dev/a/foo.txt, which should fail");
             ASSERT_THROW(this->fs_->create_file("/dev/a/foo.txt", "10MB"), sgfs::FileSystemException);

test/path_util_test.cpp

@@ -26,8 +26,8 @@ TEST_F(PathUtilTest, PathSimplification)  {
             {"./foo", "/foo"}
     };
 
-    for (const auto &test_item : input_output) {
-        MY_ASSERT_EQ(sgfs::PathUtil::simplify_path_string(test_item.first), test_item.second, test_item.first);
+    for (const auto &[input, output] : input_output) {
+        MY_ASSERT_EQ(sgfs::PathUtil::simplify_path_string(input), output, input);
     }
 }
 
@@ -66,12 +66,12 @@ TEST_F(PathUtilTest, PathAtMountPoint) {
     };
 
     for (const auto &test_item: input_output) {
-        const 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");
             MY_ASSERT_EQ(output, path_at_mp,  "{" + path + ", " + mp + "}");
-        } catch (std::logic_error &e) {
+        } catch (std::logic_error&) {
             MY_ASSERT_EQ(output, "Exception", "{" + path + ", " + mp + "}");
         }
     }