-
Notifications
You must be signed in to change notification settings - Fork 78
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 list_files function #615 #615
base: develop-pros-4
Are you sure you want to change the base?
Changes from 14 commits
7fa0947
40aa222
90e0bba
a8ed32b
4983f3b
c80a4aa
ba4a55b
da47f28
3d41ca6
4247fb1
8df65db
f4aa620
db8c2ee
a4bc889
490a3ca
b7b773e
1c42992
fc2a72f
b36e01c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,3 +17,15 @@ | |
int32_t usd_is_installed(void) { | ||
return vexFileDriveStatus(0); | ||
} | ||
static const int FRESULTMAP[] = {0, EIO, EINVAL, EBUSY, ENOENT, ENOENT, EINVAL, EACCES, // FR_DENIED | ||
EEXIST, EINVAL, EROFS, ENXIO, ENOBUFS, ENXIO, EIO, EACCES, // FR_LOCKED | ||
ENOBUFS, ENFILE, EINVAL}; | ||
|
||
int32_t usd_list_files_raw(const char* path, char* buffer, int32_t len) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this might be handled by vexFileDirectoryGet already, but can we add nullchecks for these parameters? |
||
FRESULT result = vexFileDirectoryGet(path, buffer, len); | ||
if (result != F_OK) { | ||
errno = FRESULTMAP[result]; | ||
return PROS_ERR; | ||
} | ||
return PROS_SUCCESS; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,5 +21,71 @@ 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for our C++ API please use std::string and .c_str to call the internal C funcs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make sure all of the std::strings are also references pls* There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. std::string_view There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😖 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 | ||
char *buffer = (char *) malloc(10000); | ||
if (buffer == NULL) { | ||
// try again smaller buffer to see if that works | ||
buffer = (char *) malloc(500); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keeping a |
||
if (buffer == NULL) { | ||
// if still fails, return vector containing error state | ||
// set errno to ENOMEM | ||
errno = ENOMEM; | ||
files.push_back("not enough memory to get file names"); | ||
return files; | ||
} | ||
} | ||
// Call the C function | ||
int32_t success = usd_list_files_raw(path, buffer, 10000); | ||
// Check if call successful, if error return vector containing error state | ||
if (success == PROS_ERR) { | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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, index = 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. C++ footgun alert!! |
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure where you got There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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); | ||
// Add token to files vector | ||
files.emplace_back(file_name); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it happens via an implicit conversion, it would be nice to have a comment here noting that this is the point where the string containing the file name is copied into its own |
||
// 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