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.

Sign up for GitHub

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 list_files function #615 #615

Open
wants to merge 19 commits into
base: develop-pros-4
Choose a base branch
from
Open

✨Add list_files function #615 #615

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
7fa0947
Add list_files
noam987 Oct 23, 2023
40aa222
renamed c function
noam987 Oct 24, 2023
90e0bba
Clarify the docs
noam987 Oct 24, 2023
a8ed32b
Changed list_files function in vdml_usd.cpp to return vector of std:s…
Gracelu128 Oct 27, 2023
4983f3b
Added #include <vector>
Gracelu128 Oct 27, 2023
c80a4aa
Readded original func as list_files_raw, made sure to update func in …
Gracelu128 Oct 27, 2023
ba4a55b
Altered how list_files func deals with error
Gracelu128 Oct 27, 2023
da47f28
Updated header regarding example for list_files func
Gracelu128 Oct 27, 2023
3d41ca6
Edits to list_files func, function header
Gracelu128 Oct 30, 2023
4247fb1
Edits to list_files function
Gracelu128 Nov 2, 2023
8df65db
Update header for list_files and list_files_raw function
Gracelu128 Nov 2, 2023
f4aa620
List_files dealt with possible memory allocation error
Gracelu128 Nov 3, 2023
db8c2ee
edit dealing with memory allocation error in list_files
Gracelu128 Nov 3, 2023
a4bc889
Update on dealing with errors
Gracelu128 Nov 3, 2023
490a3ca
Made stylistic edits to list_files function
Gracelu128 Nov 4, 2023
b7b773e
list_files function
Gracelu128 Nov 10, 2023
1c42992
Edits to list_files function on handling file paths, inclusion of "us…
Gracelu128 Nov 10, 2023
fc2a72f
Updated list_files function
Gracelu128 Oct 23, 2024
b36e01c
Update misc.h
Gracelu128 Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions include/pros/misc.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,7 +577,7 @@ std::int32_t list_files_raw(const char* path, char* buffer, int32_t len);

/**
* Lists the files in a directory specified by the path
* Puts the list of file names (NOT DIRECTORIES) into a vector of std::string
* Puts the list of file paths (NOT DIRECTORIES) into a vector of std::string
*
* This function uses the following values of errno when an error state is
* reached:
Expand All @@ -596,20 +596,17 @@ std::int32_t list_files_raw(const char* path, char* buffer, int32_t len);
*
* \param path
* The path to the directory to list files in
*
* \note use a path of "\" to list the files in the main directory NOT "/usd/"
* DO NOT PREPEND YOUR PATHS WITH "/usd/"
*
* \return vector of std::string of file names, if error occurs, returns vector containing
* two elements, first element is "ERROR" and second element is the error message
*
* \b Example
* \code
* void opcontrol() {
* // Will return vector containing names of files in root directory
* std::vector<std::string> files = pros::usd::list_files("/");
* // Will return vector containing file paths of files in root directory
* std::vector<std::string> files = pros::usd::list_files("/test");
* pros::delay(200);
* // Given vector of std::string file names, print each file name
* // Given vector of std::string file paths, print each file path
* // Note that if there is an error, the vector will contain two elements,
* // first element is "ERROR" and second element is the error message
*
Expand All @@ -619,7 +616,8 @@ std::int32_t list_files_raw(const char* path, char* buffer, int32_t len);
* }
* else {
* // file list returned is valid
* // Print each file name
* // Print each file path
* // Each file path in format "/usd/path/file_name"
* for (std::string& file : files) {
* std::cout << file << std::endl;
* }
Expand Down
15 changes: 14 additions & 1 deletion src/devices/vdml_usd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,19 @@ std::vector<std::string> list_files(const char* path) {
return files;
}
}

// Check path user passed in
std::string_view path_sv(path);
size_t found = path_sv.find("usd");
if (found == 0 || found == 1) {
// Deal with when user prepends path with usd
// as either "usd/..." or "/usd/..."
path_sv.remove_prefix(3);
}
Copy link
Contributor

@djava djava Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does hardcoding the 3 here deal with both cases? Wouldn't it be 3 + found?

Also to avoid the magic-number-ness of this 3, I'd probably write this as:

constexpr std::string_view usd_prefix {"usd"};
std::string_view path_sv {path};
const size_t usd_prefix_idx = path_sv.find(usd_prefix);
if (usd_prefix_idx == 0 || usd_prefix_idx == 1) {
    path_sv.remove_prefix(usd_prefix.length() + usd_prefix_idx);
}

Copy link
Contributor

@djava djava Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also what happens here if they don't include the usd prefix? We just make it work the same as if they did? May be weird, since we also always return paths prefixed with /usd/.

My preference here would be to return a "path not found" error, as just assuming they wanted an implicit /usd/ makes doing anything with a VFS outside of /usd/ in the future into a backwards compatibility break. It also improves consistency, since nothing else in PROS would accept that path.


// set path to path_sv.data()
path = path_sv.data();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

path_sv is already pointing to *path. It's a read-only "view" over a string that exists elsewhere in memory, hence the name.


// Call the C function
int32_t success = usd_list_files_raw(path, buffer, buffer_size);
// Check if call successful, if error return vector containing error state
Expand Down Expand Up @@ -76,7 +89,7 @@ std::vector<std::string> list_files(const char* path) {
file_name = std::string_view(str.data() + index, delimiter_pos - index);
// This is point where content of the std::string_view file name is copied to its
// own std::string and added to the files vector
files.emplace_back(file_name);
files.emplace_back("/usd" + std::string(path) + "/" + std::string(file_name));
Copy link
Contributor

@djava djava Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't do it like this, this creates 7 different std::strings (each of which "may" allocate and pretty likely at least 2 will, wasting a lot of performance).
I don't believe the version of libstdc++ (the standard library) PROS uses supports std::format but if so you could write the very nice:

files.emplace_back(std::format("/{}/{}", path, file_name));

if not i would recommend something more like

const size_t path_size = 1 + usd_prefix.length() + path.length() + 1 + file_name.length(); // +1's for the slashes
std::string buf;
buf.reserve(path_size); // Reserve enough space in the internal array that we can snprintf right into it

// The %.*s format specifier allows passing the size of a format arg before the value of the arg, so it lets
// us use the string_view into `str` with the C-style snprintf function even though it does not have the
// null terminator that would usually be required to know when a string ends.
// We also add 1 to path_size for the `n` argument for the null terminator, the space for which is
// implicitly included by the std::string::reserve call
std::snprintf(buf.data(), path_size + 1, "/%s/%.*s", path, static_cast<int>(file_name.length()), file_name.data());

files.push_back(std::move(buf));

This retains the property of the old version that it only had to allocate the string once.

(EDIT: removed the + 1 in path_size calculation for null terminator, as I realized that std::string::reserve implicitly adds 1 for that by itself, and add it back in snprintf)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe the version of libstdc++ (the standard library) PROS uses supports std::format

Sadly it does not :(

Copy link
Contributor

@djava djava Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gracelu128 Ty but

  1. I would recommend keeping the comments (or rewriting them in your own words if you want). Always better to explain what you're doing, especially if it may be non-obvious why (and I think readers of this code may not immediately understand what the %.*s does, and why its needed. I know I wouldn't know what that format specifier means at first glance)
  2. Sorry, I actually made an edit to the code block after you looked at it I think:

(EDIT: removed the + 1 in path_size calculation for null terminator, as I realized that std::string::reserve implicitly adds 1 for that by itself, and add it back in snprintf)

You could also just subtract 1 from the param to the reserve call if you think thats preferable.

// Increment index to start substr from
index = delimiter_pos + 1;

Expand Down