Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STD Filesystem VFS #495

Merged

Conversation

TomAtkinsonArm
Copy link
Contributor

@TomAtkinsonArm TomAtkinsonArm commented Jul 10, 2022

Description

Replaces OS specific filesystem implementations with std filesystem

This is significantly slower when it comes to enumerating files or folders. Likely due to the conversion between path and string. For now I have added a TODO for future reference

@TomAtkinsonArm TomAtkinsonArm added the framework This is relevant to the framework label Jul 10, 2022
@TomAtkinsonArm TomAtkinsonArm self-assigned this Jul 10, 2022
@TomAtkinsonArm TomAtkinsonArm linked an issue Jul 10, 2022 that may be closed by this pull request
components/vfs/include/components/vfs/helpers.hpp Outdated Show resolved Hide resolved
components/vfs/src/helpers.cpp Outdated Show resolved Hide resolved
components/vfs/src/helpers.cpp Outdated Show resolved Hide resolved
components/vfs/src/root_file_system.cpp Outdated Show resolved Hide resolved
components/vfs/src/root_file_system.cpp Outdated Show resolved Hide resolved
components/vfs/src/std_filesystem.cpp Show resolved Hide resolved
components/vfs/src/std_filesystem.cpp Show resolved Hide resolved
components/vfs/src/std_filesystem.cpp Outdated Show resolved Hide resolved
components/vfs/src/std_filesystem.cpp Outdated Show resolved Hide resolved
components/vfs/src/std_filesystem.cpp Outdated Show resolved Hide resolved
@TomAtkinsonArm TomAtkinsonArm force-pushed the std_filesystem branch 2 times, most recently from 0e3fbed to d1fc5e2 Compare July 21, 2022 13:22
more tests and fixes

recursive tests

more tests

remove order specific checks from tests

review comments
components/vfs/src/root_file_system.cpp Outdated Show resolved Hide resolved
components/vfs/src/root_file_system.cpp Outdated Show resolved Hide resolved
components/vfs/src/helpers.cpp Show resolved Hide resolved
@asuessenbach
Copy link
Contributor

Well, the PR actually LGTM.
I have a few questions/issues on the surroundings, though. Please sorry, if that feels too extensive:

  • what's those class Blob and class StdBlob in filesystem.hpp for?
    especially Blob::empty(): can there be the case that size() gives some positive value, but data() is nullptr?
    If some abstraction for a blob is needed, maybe something like using Blob = std::vector<uint8_t>; would be sufficient?
  • The argument to the functions folder_exists should be name folder_path
  • Why is the blob passed into read_chunk and readFile a std::sharedPtr<Blob> *, not just a Blob *
  • Why is FileSystem::readFile virtual? Why does RootFileSystem override it?
  • Why are those three enumerate-functions members of RootFileSystem, not FileSystem?
  • Most of the functions of FileSystem and RootFileSystem could be const.
  • In helpers.cpp, get_file_name: the second argument to substr is the a count, which defaults to npos. That is, as you want to get all the rest right of '/', you can omit it. Any maybe you should add a comment that std::string::npos == -1, so you get the complete string if there's no '/' in path.
  • In helpers.cpp, sanitize, maybe the remove trailing / part could be coded like this:
if (!sanitized.empty() && sanitized.back() == '/')
{
    sanitized.pop_back();
}
  • In root_file_system.cpp, StdBlob::data() could be implemented as return static_cast<const void *>(buffer.data());
  • In root_file_system.cpp, RootFileSystem::enumerate_files(), you should *files = std::move(all_files); if the extension is empty. And you might use helpers::get_file_extension() in the loop there.
  • In root_file_system.cpp, mount, maybe like this:
auto it = std::find_if(m_mounts.begin(), m_mounts.end(), [&file_path](auto const & mount){ return mount.first == file_path; });
if ( it != m_mounts.end())
{
    it->second = file_system;
}
else
{
    m_mounts.push_back({file_path, file_system});
}
  • In root_file_system.cpp, unmount, maybe like this:
auto it = std::find_if(m_mounts.begin(), m_mounts.end(), [&file_path](auto const & mount){ return mount.first == file_path; });
if ( it != m_mounts.end())
{
    m_mounts.erase(it);
}
  • In std_filesystem.cpp, folder_exists and file_exists: do you really want to add file_path to the m_base_path?

@TomAtkinsonArm
Copy link
Contributor Author

Merging so that #506 may be applied on top

@TomAtkinsonArm TomAtkinsonArm merged commit 6d26384 into KhronosGroup:framework/v2.0 Jul 29, 2022
TomAtkinsonArm added a commit to TomAtkinsonArm/Vulkan-Samples that referenced this pull request Jul 29, 2022
* std filesystem

more tests and fixes

recursive tests

more tests

remove order specific checks from tests

review comments

* fix duplicated check

* review changes
TomAtkinsonArm added a commit to TomAtkinsonArm/Vulkan-Samples that referenced this pull request Nov 21, 2022
* std filesystem

more tests and fixes

recursive tests

more tests

remove order specific checks from tests

review comments

* fix duplicated check

* review changes
TomAtkinsonArm added a commit to TomAtkinsonArm/Vulkan-Samples that referenced this pull request Nov 21, 2022
* std filesystem

more tests and fixes

recursive tests

more tests

remove order specific checks from tests

review comments

* fix duplicated check

* review changes
TomAtkinsonArm added a commit that referenced this pull request Nov 21, 2022
* window interface

* headless window

* GLFW window

* add VKB_TEST_GLFW

* fix glfw errors

* add samples_launcher window improvements

* STD Filesystem VFS (#495)

* std filesystem

more tests and fixes

recursive tests

more tests

remove order specific checks from tests

review comments

* fix duplicated check

* review changes

* GLFW window

* add VKB_TEST_GLFW

* directly create a surface

* Direct to Display
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework This is relevant to the framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FI: Use std::filesystem
4 participants