1
\$\begingroup\$

win_xept.hpp - a basic windows api exception

#include "../utl_macro.hpp"

namespace xept
{
    using win_t = struct utl_exception // utl_exception randomly generates a name
                                       // this hides exception names from rtti
    {
        DWORD e = GetLastError( );
    };
}

ramfile.hpp - an interface for any type of file where the data is needed in ram

#pragma once

class ramfile_t
{
public:
    virtual std::uintmax_t size( ) = 0;
    virtual std::vector< std::byte > raw( ) = 0;
    virtual void overwrite( std::vector< std::byte > const & raw,
                            std::size_t const & off = 0 ) = 0;
    virtual void set( std::vector< std::byte > const & raw ) = 0;
};

ramfile_xept.hpp - the basic ramfile exception

#include "../win_xept.hpp"

namespace xept
{
    using ramfile_t = struct utl_exception:
        win_t
    { };
}

ramfile_disk.hpp - basic file for disk

class ramfile_disk_t:
    public ramfile_t
{
public:
    ramfile_disk_t( std::filesystem::path path ):
        path( std::move( path ) ),
        file_guard( file_locks[ this->path.c_str( ) ] )
    {
        assert( path.is_absolute( ) );
    }

    std::uintmax_t size( ) final
    {
        return std::filesystem::file_size( path );
    }

    std::vector< std::byte > raw( ) final
    {
        std::basic_ifstream< std::byte > file( path,
                                               std::ofstream::binary );

        assert( file.is_open( )
                && file.good( ) );

        file.seekg( 0 );

        return std::vector< std::byte >( std::istreambuf_iterator< std::byte >( file ),
                                         std::istreambuf_iterator< std::byte >( ) );
    }

    void set( std::vector< std::byte > const & data ) final
    {
        std::basic_ofstream< std::byte > file( path,
                                               std::ofstream::binary | std::ofstream::trunc );

        assert( file.is_open( )
                && file.good( ) );

        file.write( data.data( ),
                    data.size( ) );
    }

    void overwrite( std::vector< std::byte > const & data,
                    std::size_t const & off = 0 ) final
    {
        std::basic_ofstream< std::byte > file( path,
                                               std::ofstream::binary );

        assert( file.is_open( )
                && file.good( ) );

        file.seekp( off ).write( data.data( ),
                                 data.size( ) );
    }

    void set_attr( bool readonly,
                   bool hidden )
    {
        DWORD attr = 0;

        if ( readonly )
            attr |= FILE_ATTRIBUTE_READONLY;
        if ( hidden )
            attr |= FILE_ATTRIBUTE_HIDDEN;

        if ( 0 == SetFileAttributes( path.c_str( ),
                                     attr
                                         ? attr
                                         : FILE_ATTRIBUTE_NORMAL ) )
            throw xept::ramfile_disk_attr_t( );
    }

protected:
    std::filesystem::path path;

private:
    using file_locks_t = std::unordered_map< std::wstring_view,
                                             std::mutex >;
    static file_locks_t file_locks;

    std::lock_guard< file_locks_t::mapped_type > file_guard;
};

ramfile_disk_xept.hpp - exceptions throwable from ramfile_disk

#include "ramfile_xept.h"

namespace xept
{
    using ramfile_disk_attr_t = struct utl_exception:
        ramfile_t
    { };
}
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Design: The design is pretty bad. Think more on how you plan to use it. Currently, it is prone to deadlocks.

Say, you create two ramfile_disk_t instances in two different places in your code of the same file. The second instance will be blocked until first is finished. Is this really what you want? Is this the only way you want the code to be used?

At least add options like:

  1. "shared usage" - if you only want to read, there is no reason to disable access from several places. Also user may want to manage read/write safety themselves instead of relying on crude methods.
  2. "try open" - return false if locked.
  3. "open" - throw if locked.
  4. "wait for open" - waits until unlocked if locked.
  5. "lock on usage" - lock/unlocks the mutex upon read/write instead of creation/destruction of the class.

Technical Issues:

  1. Avoid #include "../win_xept.hpp". Imagine there is another win_xept.hpp file somewhere and user added an include path that results in loading the wrong win_xept.hpp. This leads to build corruption. If anything you'd better make an extra folder for all your code files just to be extra safe. So you always include #include "my_codebase/current_project/win_xept.hpp". As a bonus your code will be organized better.

  2. Frequently one makes a static create function for abstract classes that will instantiate the implementation depending on user's request. This way, user at no point needs to include implementation's headers.

  3. ramfile_t is a class with virtual functions. Therefore, it is ought to have a virtual destructor (virtual ~ramfile_t() = default). This is especially true for purely abstract classes. Also, do you really think you'll have more that one implementation of the class, why even bother with the interface?

  4. ramfile_disk_t has data-racing issues with the static member file_locks. You ought to guard it with a mutex each time you access it.

  5. I am not 100% sure, but most likely

    std::vector< std::byte >( std::istreambuf_iterator< std::byte >( file ),
                              std::istreambuf_iterator< std::byte >( ) );
    

    is a very slow method to read the file. Consider using more low-level functions like fread or read of the iostream instead of the iterator-based approach. At least make some tests.

  6. It is a bit nitpicky, but is there any reason to pass std::size_t const & off = 0 as const reference in the overwrite method? Just pass it by value std::size_t off; not only passing by reference is slower, it becomes nuisance once you attempt to schedule the function in an asynchronous thread.

\$\endgroup\$
3
  • \$\begingroup\$ Dunno how to fix 1, I'd have to make the includes non-portable. I don't think including implementation headers is too bad for #2. I've fixed #3. I've tested #5, and it's all optimized the same in release. Got any suggestions for how to implement #4? I use const & for #6 because I've heard it allows the compiler to inline more readily. I have each file locked because the filesystem is very volatile and dangerous by nature, and this is my internally-safe solution. \$\endgroup\$
    – user226075
    Commented Jul 11, 2020 at 10:46
  • \$\begingroup\$ @superdeveloper "Dunno how to fix 1..." what? Every single open source library does so. When you include boost it is always "boost/..." and it is totally portable. About 6# - it doesn't matter for such variables and methods. Don't use const & when it is out of place. About #4 - wrap obtaining mutex into a safe-thread static private function and call the method in the constructor. #2 that's one of the purpose of abstract classes. It allows you to hide implementation details. \$\endgroup\$
    – ALX23z
    Commented Jul 11, 2020 at 13:23
  • \$\begingroup\$ @superdeveloper while filesystem has issues, it is not a good idea for class to fully lock usage of the file for its whole existance. Your operations are already macro-level. Wouldn't it make more sense to lock the file for the duration of an operation? Furthermore, lock_guard/mutex must be released by the same thread - which doesn't fit with file usage. Mutexes should be used for thread synchronization - not file synchronization. \$\endgroup\$
    – ALX23z
    Commented Jul 11, 2020 at 16:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.