-
Notifications
You must be signed in to change notification settings - Fork 77
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 8 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 |
---|---|---|
|
@@ -26,6 +26,7 @@ | |
|
||
#include <cstdint> | ||
#include <string> | ||
#include <vector> | ||
|
||
namespace pros { | ||
inline namespace v5 { | ||
|
@@ -525,6 +526,95 @@ namespace usd { | |
* \endcode | ||
*/ | ||
std::int32_t is_installed(void); | ||
|
||
/** | ||
Lists the files in a directory specified by the path | ||
* Puts the list of file names (NOT DIRECTORIES) into the buffer seperated by newlines | ||
* | ||
* This function uses the following values of errno when an error state is | ||
* reached: | ||
* | ||
* EIO - Hard error occured in the low level disk I/O layer | ||
* EINVAL - file or directory is invalid, or length is invalid | ||
* EBUSY - THe physical drinve cannot work | ||
* ENOENT - cannot find the path or file | ||
* EINVAL - the path name format is invalid | ||
* EACCES - Access denied or directory full | ||
* EEXIST - Access denied | ||
* EROFS - SD card is write protected | ||
* ENXIO - drive number is invalid or not a FAT32 drive | ||
* ENOBUFS - drive has no work area | ||
* ENFILE - too many open files | ||
* | ||
* | ||
* | ||
* \note use a path of "\" to list the files in the main directory NOT "/usd/" | ||
* DO NOT PREPEND YOUR PATHS WITH "/usd/" | ||
* | ||
* \return 1 on success or PROS_ERR on failure, setting errno | ||
* | ||
* \b Example | ||
* \code | ||
* void opcontrol() { | ||
* char* test = (char*) malloc(128); | ||
* pros::usd::list_files_raw("/", test, 128); | ||
* pros::delay(200); | ||
* printf("%s\n", test); //Prints the file names in the root directory seperated by newlines | ||
* pros::delay(100); | ||
* pros::list_files_raw("/test", test, 128); | ||
* pros::delay(200); | ||
* printf("%s\n", test); //Prints the names of files in the folder named test seperated by newlines | ||
* pros::delay(100); | ||
* } | ||
* \endcode | ||
*/ | ||
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 the buffer seperated by newlines | ||
* | ||
* This function uses the following values of errno when an error state is | ||
* reached: | ||
* | ||
* EIO - Hard error occured in the low level disk I/O layer | ||
* EINVAL - file or directory is invalid, or length is invalid | ||
* EBUSY - THe physical drinve cannot work | ||
* ENOENT - cannot find the path or file | ||
* EINVAL - the path name format is invalid | ||
* EACCES - Access denied or directory full | ||
* EEXIST - Access denied | ||
* EROFS - SD card is write protected | ||
* ENXIO - drive number is invalid or not a FAT32 drive | ||
* ENOBUFS - drive has no work area | ||
* ENFILE - too many open files | ||
* | ||
* | ||
* | ||
* \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 | ||
* one element specifying the error state | ||
* | ||
* \b Example | ||
* \code | ||
* void opcontrol() { | ||
* char* test = (char*) malloc(128); | ||
* // Will return vector containing names of files in root directory | ||
* std::vector<std::string> files = pros::usd::list_files("/", test, 128); | ||
* pros::delay(200); | ||
* // Given vector of std::string file names, print each file name | ||
* // Note that if there is an error, the vector will contain one element, which | ||
* // is the error state | ||
* // Print each file name | ||
* for (std::string file : files) { | ||
* std::cout << file << std::endl; | ||
* } | ||
* } | ||
* \endcode | ||
*/ | ||
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. Surely the docs should explain the params as well? 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::vector<std::string> list_files(const char* path, char* buffer, int32_t len); | ||
} // namespace usd | ||
|
||
} // namespace pros | ||
|
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,80 @@ 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, 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 function should definitely not require users to pass in their own 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. Implemented this change in commit 4247fb1 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. Looks good, can I ask if the number 10000 came from somewhere or was it chosen arbitrarily? 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. Came from an estimate of the worst case that is realistic to happen. We needed to chose a number, and decided that 100 files in a single directory with an approximate average length of 100 (really 99 if you include the separators) will be enough for every feasible use case we could come up with. 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. Ok, fair enough. If you're choosing a number, a power of 2 (or sum of powers of 2) might be nice for memory allocator friendliness :) (I have no idea what allocation strategy VexOS uses, this may be completely irrelevant) 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. use a std::string instead of a c_string 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 actually |
||
std::vector<std::string> files = {}; | ||
// Call the C function | ||
int32_t success = usd_list_files_raw(path, buffer, len); | ||
// 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) |
||
switch (errno) { | ||
case EIO: | ||
files.push_back("EIO"); | ||
break; | ||
case EINVAL: | ||
files.push_back("EINVAL"); | ||
break; | ||
case EBUSY: | ||
files.push_back("EBUSY"); | ||
break; | ||
case ENOENT: | ||
files.push_back("ENOENT"); | ||
break; | ||
case EACCES: | ||
files.push_back("EACCES"); | ||
break; | ||
case EEXIST: | ||
files.push_back("EEXIST"); | ||
break; | ||
case EROFS: | ||
files.push_back("EROFS"); | ||
break; | ||
case ENXIO: | ||
files.push_back("ENXIO"); | ||
break; | ||
case ENOBUFS: | ||
files.push_back("ENOBUFS"); | ||
break; | ||
case ENFILE: | ||
files.push_back("ENFILE"); | ||
break; | ||
default: | ||
// If none of the above, will be FILE IO error | ||
files.push_back("FILE I/O ERROR"); | ||
break; | ||
} | ||
return files; | ||
} | ||
// Parse buffer given call successful, split by '/n' | ||
// Store char * buffer as std::string in str | ||
// Store delimiter in std::string, '\n' | ||
std::string str(buffer); | ||
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. There's no reason to allocate a whole additional 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. Implemented this change in commit 3d41ca6. |
||
std::string delimiter = "\n"; | ||
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. char delimeter = '\n'; You definitely don't need a whole 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. Implemented this change in commit 3d41ca6 |
||
|
||
// position to keep track of position in str | ||
// file_name used to store each file name | ||
size_t position = 0; | ||
std::string file_name; | ||
|
||
// Loop until delimiter '\n' can not be found anymore | ||
while ((position = str.find(delimiter)) != std::string::npos) { | ||
// file_name is the string from the beginning of str to the first '\n', excluding '\n' | ||
file_name = str.substr(0, position); | ||
// Add token to files vector | ||
files.push_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. You should move file_name into the vector instead of copying it. This will reuse the already allocated heap buffer instead of copying over a new one, resulting in each file name being copied once instead of twice or more. files.emplace_back(std::move(file_name)); Alternatively, 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. Implemented this change in commit 3d41ca6. Made file_name a std::string_view. |
||
// Erase file_name from str, would be pos + 1 if we wanted to include '\n' | ||
str.erase(0, position + 1); | ||
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 won't work if you use a 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. Implemented this change in commit 3d41ca6. Altered code to instead have index to keep track of start for next iteration. 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. Very nice! Thanks |
||
} | ||
|
||
// 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.
for (std::string& file : files) {
That
&
is very important for performance reasons.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.
Implemented this suggestion in commit on Oct 30, 3d41ca6