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.

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
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
19 changes: 10 additions & 9 deletions include/pros/misc.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -549,24 +549,25 @@ Lists the files in a directory specified by the path
* \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
* \return vector of std::string of file names, if error occurs, returns vector containing
* one element, PROS_ERR
*
* \b Example
* \code
* void opcontrol() {
* char* test = (char*) malloc(128);
* pros::usd::list_files("/", test, 128);
* std::vector<std::string> files = list_files("/", 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("/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);
* // Given vector of std::string file names, print each file name
* // Note that if there is an error, the vector will contain one element, PROS_ERR
* // Print each file name
* for (std::string file : files) {
Copy link
Contributor

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.

Copy link
Contributor Author

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

* std::cout << file << std::endl;
* }
* }
* \endcode
*/
Copy link
Contributor

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?

Copy link
Contributor Author

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

std::int32_t list_files(const char* path, char* buffer, std::int32_t len);
std::vector<std::string> list_files(const char* path, char* buffer, int32_t len);
} // namespace usd

} // namespace pros
Expand Down
35 changes: 33 additions & 2 deletions src/devices/vdml_usd.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,39 @@ std::int32_t is_installed(void) {
return usd_is_installed();
}

int32_t list_files(const char* path, char* buffer, int32_t len) {
return usd_list_files(path, buffer, len);
std::vector<std::string> list_files(const char* path, char* buffer, int32_t len) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function should definitely not require users to pass in their own buffer and len. You should just allocate a buffer yourself at the top of this function body then free it after it's done being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented this change in commit 4247fb1

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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)

Copy link
Member

Choose a reason for hiding this comment

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

use a std::string instead of a c_string pls.

Copy link
Contributor

Choose a reason for hiding this comment

The 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(path, buffer, len);
// Check if call successful, if PROS_ERR return vector containing PROS_ERR
if (success == PROS_ERR) {
// push_back PROS_ERR to files vector
files.push_back(std::to_string(success));
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no reason to allocate a whole additional std::string for this, use std::string_view over the already-existing buffer instead. You can still use .find() the same way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented this change in commit 3d41ca6.

std::string delimiter = "\n";
Copy link
Contributor

Choose a reason for hiding this comment

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

char delimeter = '\n';

You definitely don't need a whole std::string for just \n, which is only a single char.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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, file_name could be a std::string_view that you emplace_back. Since files::value_type is std::string, that will result in the file name being copied (for the first time). Actually I think I like this solution better than the move anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work if you use a std::string_view for str (and it's very unnecessarily slow even as a std::string). You should either keep track of the index to start at for the next iteration, or maybe str = str.substr(...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice! Thanks

}

// return vector of file names
return files;
}

} // namespace usd
Expand Down
Loading