3
\$\begingroup\$

As a side-project, I'm trying to implement a distributed object storage. I've decided to use C++ and libuv, two technologies I had never played with, since it gave me a good reason to learn them.

For this project, I needed an abstraction over the filesystem. This was required in order to use libuv as a backend. This was also a good way to learn how C++11 behaves.

However, I'm alone on this project, and I'm trying to figure out whether I use templates, lvalues, rvalues, universal pointers etc. correctly. It's better to break the bad practices early.

Could I have a code review on it? I've linked the repository.

https://github.com/objd/vfs

By the way, it's not completed. I still need to abstract file descriptors, stat structures and provide type-erased containers for them. I'm also very late on unit tests. However, I'd still like a code review on what has been done so far.

The two main interfaces are path and filesystem. I've put their definitions and the declaration of all implementations. In the future, I want to add an interface for stat, fd and buffer.

class path
{
  public:

    virtual ~path() noexcept
    {};

    virtual bool is_root() const noexcept = 0;
    virtual bool is_absolute() const noexcept = 0;
    virtual bool is_relative() const noexcept = 0;
    virtual bool is_valid() const noexcept = 0;

    virtual const std::string &str() const noexcept = 0;

    virtual path &prepend(path &path) noexcept = 0;
    virtual path &prepend(std::string &str) noexcept = 0;
    virtual path &prepend(std::string &&str) noexcept = 0;

    virtual path &append(path &path) noexcept = 0;
    virtual path &append(std::string &str) noexcept = 0;
    virtual path &append(std::string &&str) noexcept = 0;

    virtual path &parent() noexcept = 0;
    virtual path &filename() noexcept = 0;
};


template<typename t_path>
class base_path : public path;

class any_path : public base_path<any_path>;

class unix_path : public base_path<unix_path>;

template<typename t_path, typename t_stat, typename t_file, typename t_buffer>
class filesystem
{
  public:
    using exists_cb = std::function<
        void(t_path &, int, bool)>;

    using stat_cb = std::function<
        void(t_path &, int, t_stat)>;

    using mkdir_cb = std::function<
        void(t_path &, int)>;

    using mkdirs_cb = std::function<
        void(t_path &, int)>;

    using create_cb = std::function<
        void(t_path &, int)>;

    using move_cb = std::function<
        void(t_path &, t_path &, int)>;

    using copy_cb = std::function<
        void(t_path &, t_path &, int)>;

    using link_cb = std::function<
        void(t_path &, t_path &, int)>;

    using symlink_cb = std::function<
        void(t_path &, t_path &, int)>;

    using unlink_cb = std::function<
        void(t_path &, int)>;

    using open_cb = std::function<
        void(t_path &, int, t_file &)>;

    using fstat_cb = std::function<
        void(t_file &, int, t_stat)>;

    using read_cb = std::function<
        void(t_file &, int, vfs::buffer &)>;

    using write_cb = std::function<
        void(t_file &, int, vfs::buffer &)>;

    using truncate_cb = std::function<
        void(t_file &, int, uint64_t)>;

    using close_cb = std::function<
        void(t_file &, int)>;

    virtual ~filesystem() noexcept 
    {};

    virtual int exists(t_path &&path, exists_cb cb) noexcept = 0;
    virtual int stat(t_path &&path, stat_cb cb) noexcept = 0;
    virtual int mkdir(t_path &&path, int32_t mode, mkdir_cb cb) noexcept = 0;
    virtual int mkdirs(t_path &&path, int32_t mode, mkdirs_cb cb) noexcept = 0;
    virtual int create(t_path &&path, int32_t mode, create_cb cb) noexcept = 0;
    virtual int move(t_path &&path, t_path &&move_path, move_cb cb) noexcept = 0;
    virtual int copy(t_path &&path, t_path &&copy_path, copy_cb cb) noexcept = 0;
    virtual int link(t_path &&path, t_path &&other_path, link_cb cb) noexcept = 0;
    virtual int symlink(t_path &&path, t_path &&other_path, symlink_cb cb) noexcept = 0;
    virtual int unlink(t_path &&path, unlink_cb cb) noexcept = 0;
    virtual int open(t_path &&path, int32_t mode, int32_t flags, open_cb cb) noexcept = 0;
    virtual int stat(t_file &file, fstat_cb cb) noexcept = 0;
    virtual int read(t_file &file, t_buffer &&buf, off64_t off, read_cb cb) noexcept = 0;
    virtual int write(t_file &file, t_buffer &&buf, off64_t off, write_cb cb) noexcept = 0;
    virtual int truncate(t_file &file, uint64_t size, off64_t off, truncate_cb cb) noexcept = 0;
    virtual int close(t_file &file, close_cb cb) noexcept = 0;
};

class uv_filesystem : public filesystem<vfs::any_path, uv_stat_t, uv_file, uv_buf_t>;
\$\endgroup\$
0
4
\$\begingroup\$

Use an existing filesystem library

You probably want to use the C++17 filesystem library. It is very close to what you have posted here. If you cannot use C++17, then use the Boost Fileystem library, on which the C++17 filesystem library is based. The latter should work just fine in C++11 code.

To ensure you can easily switch from Boost to C++17 later, use a namespace alias. For the Boost library:

namespace fs = boost::filesystem;

For the C++17 library:

namespace fs = std::filesystem;

Then your code can just use fs::path, fs::create_directory() and so on.

Don't be afraid to drop your own implementation and switch to an existing filesystem library, even though it seems like this wastes perfectly good code. The effort was not wasted, you probably learned form it. But the existing libraries have much more people working on it, and will be of a higher quality. Also, you might get collaborators working on your project in the future, and they might already be familiar with the Boost or C++17 filesystem library, so they'll be more comfortable using that.

If this will really not be suitable for your project, then at least try to emulate the interface of these standard libraries as much as possible.

Async

Instead of making async versions of existing filesystem-related functions, consider just making use of C++'s std::async() function. Instead of having a async version of copy(), can you just do std::async([](){fs::copy(foo, bar);})? Again, if this does not work in your case, then at least try to emulate how std::async() works as close as possible. In particular, std::async() returns a std::future<> with the results of the function. So instead of:

using exists_cb = std::function<void(t_path &, int, bool)>;
virtual int exists(t_path &&path, exists_cb cb) noexcept = 0;

Write:

virtual std::future<bool> exists(const fs::path &path);

Use const function arguments where possible

You have a lot of functions that take an rvalue reference instead of a const reference. If you don't intend to change the argument (like the t_path arguments for the member functions in class filesystem), it is better to explicitly make them const references. There are two main advantages: you will get a compiler error when you accidentily do change an argument, and if the compiler knows you will not change the argument, it can make better optimizations of your code.

\$\endgroup\$
4
  • \$\begingroup\$ Thank you for your answer! I've considered both std::filesystem and boost::filesystem, however, they are synchronous. I'm trying to make a single-threaded design that I'll be able to scale through multi-processing and futures are usually ran on a thread-pool (even if they're only an abstraction and they could be implemented via an event loop). Would there be any benefit to futures and/or promises compared to callbacks? \$\endgroup\$ – sturcotte06 Mar 18 '18 at 16:28
  • 1
    \$\begingroup\$ There are several advantages. You get rid of a lot of custom callback types. Anything that can take a future as an argument can now work with the results of your asynchronous functions. \$\endgroup\$ – G. Sliepen Mar 18 '18 at 19:21
  • \$\begingroup\$ Most of the lvalue-reference arguments should also be const references, too. \$\endgroup\$ – Toby Speight Mar 19 '18 at 17:55
  • \$\begingroup\$ The compiler will not optimize based on the const keyword. It is not allowed to assume you won't const_cast it away at a later point. Unless it can fully inline all code, in which case it doesn't need the const qualifier: i.e. const does not affect optimization. \$\endgroup\$ – Giel Mar 20 '18 at 13:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.