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

Add new file access methods to support more use cases conveniently #49

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

technoyes
Copy link

@technoyes technoyes commented Aug 28, 2023

This change adds support for directly fetching resource data as std::string as well as raw "const char*" combined with std::size_t. This allows cleaner code in use cases where the existing file interface is overkill.

Complete with docs and tests. Also added small doc about the NULL byte that is appended to all resources.

Not super happy about the get_as_raw_ptr() returning size through an output argument, but I can't think of a cleaner way.

Thoughts?

@technoyes technoyes force-pushed the feature/additional-access-functions branch from fc3b502 to 3e1f1bb Compare August 29, 2023 13:04
@Wunkolo
Copy link

Wunkolo commented Aug 29, 2023

Not super happy about the get_as_raw_ptr() returning size through an output argument, but I can't think of a cleaner way.

Maybe std::span? Or std::string_view?
There was some discussion about this here.
#24
Preferably without doing a deep copy of any kind. An std::string will create a deep copy of the entire file's data so maybe a string_view is preferred.

@technoyes
Copy link
Author

I will send an updated PR adding std::string_view support, but make it optional since it requires C++17. I don't think std::span offers any additional value, and it also requires C++20. I think I will keep the raw_ptr interface for people stuck with C++11 compilers.

@technoyes technoyes force-pushed the feature/additional-access-functions branch from 3e1f1bb to c83f019 Compare August 31, 2023 20:40
@technoyes
Copy link
Author

I added a std::string_view accessor, updated the docs (did a drive-by fix on an indentation problem), added a standalone example and added more tests (drive-by fix of a potential undefined return value).

Separated this into five separate commits to make it easier to review.

Let me know what you think when you have time @vector-of-bool.

@technoyes technoyes changed the title Add get_as_string() and get_as_raw_ptr() Add new file access methods to support more use cases conveniently Sep 1, 2023
technoyes added 5 commits September 19, 2023 12:05
To make the resource access convenient in more cases the
following access functions have been added:

    get_as_string()
    get_as_string_view() - requires C++17
    get_as_raw_ptr()
    get_size()
@technoyes technoyes force-pushed the feature/additional-access-functions branch from c83f019 to e2b1bb8 Compare September 19, 2023 10:06
@technoyes
Copy link
Author

technoyes commented Sep 19, 2023

Fixed a problem with reference assignment (noticed it with MSVC with C++20 standard mode).

@vector-of-bool
Copy link
Owner

Sorry that I missed this. It got buried in notifications and I just saw it after your comment yesterday. This project has unfortunately languished a bit while I've been busy with other things.

I think, IIUC, what this PR and #24 are looking for is an easy contiguous_range interface. The file class gets really close, but is just missing the data() accessor. Simply adding data() to satisfy std::ranges::contiguous_range will get it a long way in terms of compatibility with contiguous-range algorithms.

As for the convenience interfaces of span/string(_view)/vector/etc., In another library, I have a buffer class with a constrained conversion operator template which allows one to explicit-convert it to string_view/span/vector-like types. I think such a conversion template may be useful for file. It is entirely possible (although a bit uglier) to write such a conversion template without C++20 concepts. Basically:

// BEWARE: Untested code!
template <
    typename To, 
    typename ConstPointer = typename To::const_pointer,
    typename = std::enable_if_t<
            // Can construct with a size-pointer pair
            std::is_constructible_v<To, ConstPointer, std::size_t>
            // Make sure the elements are byte-sized
        and sizeof(std::remove_pointer_t<ConstPointer>) == 1
    >>
explicit operator T() const noexcept {
    return To(reinterpret_cast<ConstPointer>(data()), size());
}

with this, any class which satisfies the constraints can be used in an explicit conversion:

cmrc::file get_some_file();
// ...
auto sv = std::string_view(get_some_file());

std::span, std::vector, std::string_view, QString, etc. all in one interface.

How does that sound?

@technoyes
Copy link
Author

technoyes commented Sep 20, 2023

No worries at all - mailboxes get flooded and life is busy for us all. Thank you for creating the little gem that CMRC is. Really helps creating self-contained portable apps smoothly!

That said, that is some strong C++ Kung Fu you have there! Learned a lot just looking up what those template constructs do.

From a library users perspective your solution looks clean and efficient.

Does this mean that as a consumer of the resources one could do this:

// No copy of data, direct access to read-only (const) data
const char* raw = std::string_view(fs.open("file.txt")).data();

If so, I'd happily take your solution over this MR.

@technoyes
Copy link
Author

Gently bumping this one a little bit @vector-of-bool :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants