✨Add list_files function #615 #615
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,5 +21,90 @@ std::int32_t is_installed(void) { | |
| return usd_is_installed(); | ||
| } | ||
|
|
||
| std::int32_t list_files_raw(const char* path, char* buffer, int32_t len) { | ||
|
There was a problem hiding this comment. for our C++ API please use std::string and .c_str to call the internal C funcs. There was a problem hiding this comment. make sure all of the std::strings are also references pls* There was a problem hiding this comment.
I understand that's more C++onic but for the sake of our users I think a std::string would be a little more approachable... saves everyone a google search. There was a problem hiding this comment. 😖 wasted allocation for every call with a string literal for path, which I assume is the typical case. There was a problem hiding this comment.
If you choose to use |
||
| return usd_list_files_raw(path, buffer, len); | ||
| } | ||
|
|
||
| std::vector<std::string> list_files(const char* path) { | ||
| std::vector<std::string> files = {}; | ||
| // malloc buffer to store file names | ||
| size_t buffer_size = 10000; | ||
| char *buffer = (char *) malloc(buffer_size); | ||
|
There was a problem hiding this comment. You should use the C++ It's a bit less scary type-wise and very widely considered better stylistically for C++. There was a problem hiding this comment. Actually even better would be |
||
| if (buffer == NULL) { | ||
| // try again smaller buffer to see if that works | ||
| buffer_size = 500; | ||
| buffer = (char *) malloc(buffer_size); | ||
| if (buffer == NULL) { | ||
| // if still fails, return vector containing error state | ||
| // set errno to ENOMEM | ||
| errno = ENOMEM; | ||
| files.push_back("ERROR"); | ||
| files.push_back("not enough memory to get file names"); | ||
| 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); | ||
| } | ||
|
There was a problem hiding this comment. How does hardcoding the 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);
}There was a problem hiding this comment. 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 My preference here would be to return a "path not found" error, as just assuming they wanted an implicit |
||
|
|
||
| // set path to path_sv.data() | ||
| path = path_sv.data(); | ||
|
There was a problem hiding this comment.
|
||
|
|
||
| // 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 | ||
| if (success == PROS_ERR) { | ||
| files.push_back("ERROR"); | ||
| // Check errno to see which error state occurred | ||
| // push back error state to files vector as std::string | ||
|
There was a problem hiding this comment. mmm I'm definitely not convinced that this error handling strategy is a good approach. Maybe an out-param for the vector is better so you have some other way to return errors? That's more inline with how pros usually does things anyway. (returning errors via return type) |
||
| if (errno == EINVAL || errno == ENOENT) { | ||
| // errors related to path not found | ||
| files.push_back("path not found"); | ||
| } else { | ||
| // other error stats related to file io | ||
| files.push_back("file i/o error"); | ||
| } | ||
| return files; | ||
| } | ||
|
|
||
| // Parse buffer given call successful, split by '/n' | ||
| std::string_view str(buffer); | ||
|
|
||
| // delimter_pos is the position of the delimiter '\n' | ||
| // index of which character to start substr from | ||
| // file_name used to store each file name | ||
| size_t delimiter_pos = 0; | ||
| size_t index = 0; | ||
| std::string_view file_name; | ||
|
|
||
| // Loop until delimiter '\n' can not be found anymore | ||
| while ((delimiter_pos = str.find('\n', index)) != std::string::npos) { | ||
|
There was a problem hiding this comment. If we really wanted to optimize this even more you could write a sentinel function (or whatever people call it I'm tired atm) that grabs all the delimter indexes, and then another loop that stores all the delimter positions in a data structure such as a vector, and then another loop that iterates through the data structure. This changes the O(N * M) behavior of this to O(N + M) where N is the size of the string and M is the number of delimeters. There was a problem hiding this comment. I'm not sure where you got There was a problem hiding this comment.
ah you're right, I didn't notice the index parameter last night. |
||
| // file_name is the string from the beginning of str to the first '\n', excluding '\n' | ||
| 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("/usd" + std::string(path) + "/" + std::string(file_name)); | ||
|
There was a problem hiding this comment. You can't do it like this, this creates 7 different 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 There was a problem hiding this comment.
Sadly it does not :( There was a problem hiding this comment. @Gracelu128 Ty but
You could also just subtract 1 from the param to the |
||
| // Increment index to start substr from | ||
| index = delimiter_pos + 1; | ||
|
|
||
| // If index is greater than or equal to str length, break | ||
| if (index >= str.length()) { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Free buffer | ||
| free(buffer); | ||
|
|
||
| // Return vector of file names | ||
| return files; | ||
| } | ||
|
|
||
| } // namespace usd | ||
| } // namespace pros | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surely the docs should explain the params as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added param in header for funcs in commit 3d41ca6 and 4247fb1