Skip to content

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

Open
Randalphwa wants to merge 5 commits into
mainfrom
pr/49
Open

Add new file access methods to support more use cases conveniently#21
Randalphwa wants to merge 5 commits into
mainfrom
pr/49

Conversation

@Randalphwa

Copy link
Copy Markdown
Member

Migrated from vector-of-bool/cmrc PR #49 by @technoyes

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 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()
@Randalphwa

Copy link
Copy Markdown
Member Author

(Migrated comment by @Wunkolo on 8/29/2023 5:37:28 PM from original #49)

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.
vector-of-bool/cmrc#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.

@Randalphwa

Copy link
Copy Markdown
Member Author

(Migrated comment by @technoyes on 8/31/2023 11:00:46 AM from original #49)

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.

@Randalphwa

Copy link
Copy Markdown
Member Author

(Migrated comment by @technoyes on 8/31/2023 8:50:10 PM from original #49)

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.

@Randalphwa

Copy link
Copy Markdown
Member Author

(Migrated comment by @technoyes on 9/19/2023 10:12:33 AM from original #49)

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

@Randalphwa

Copy link
Copy Markdown
Member Author

(Migrated comment by @vector-of-bool on 9/20/2023 5:04:27 PM from original #49)

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?

@Randalphwa

Copy link
Copy Markdown
Member Author

(Migrated comment by @technoyes on 9/20/2023 5:57:14 PM from original #49)

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.

@Randalphwa

Copy link
Copy Markdown
Member Author

(Migrated comment by @technoyes on 10/20/2023 1:21:54 PM from original #49)

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.

1 participant