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 an API for directory iteration #323

Merged
merged 7 commits into from
Jan 15, 2021
Merged

Add an API for directory iteration #323

merged 7 commits into from
Jan 15, 2021

Conversation

cottsay
Copy link
Member

@cottsay cottsay commented Jan 12, 2021

This API enables iteration of large directories. There are three parts here:

  1. The 3 new iteration functions
  2. Changing the existing iteration to use the new API during directory size computation
  3. The function originally presented in Add rcutils_list_directory function #249 (which is much more convenient)

It's worth discussing the reason I created the rcutils_dir_iter_state_t struct instead of putting more stuff in rcutils_dir_iter_t. The biggest motivating factor there is that we need windows.h for those types, and I REALLY REALLY don't want that polluting our rcutils headers. Additionally, the idea behind this API is to hide the platform-specific data structures, so that stuff is essentially private data for the API.

Closes #249

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • macOS-clang Build Status
  • Windows Build Status

@cottsay cottsay added enhancement New feature or request in review Waiting for review (Kanban column) labels Jan 12, 2021
@cottsay cottsay self-assigned this Jan 12, 2021
This change adds a simple function for listing the contents of a
directory, and returning the names of those files as a string_array
object.

Signed-off-by: Scott K Logan <[email protected]>
src/filesystem.c Outdated Show resolved Hide resolved
test/test_filesystem.cpp Outdated Show resolved Hide resolved
src/filesystem.c Outdated
Comment on lines 516 to 518
if (NULL != state->dir) {
closedir(state->dir);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

TIL that closedir(NULL) causes some sort of abort on macOS, but not on Linux.

This reverts commit 9837765.

Signed-off-by: Scott K Logan <[email protected]>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This is a lot of really good work; I can't see much to complain about with it.

So looks good to me with green CI. Thanks for iterating!

include/rcutils/filesystem.h Show resolved Hide resolved
include/rcutils/filesystem.h Show resolved Hide resolved
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

i think it looks good with nitpick.

include/rcutils/filesystem.h Outdated Show resolved Hide resolved
@fujitatomoya
Copy link
Collaborator

@Barry-Xu-2018 friendly ping, you might wanna take a look at this since we added rcutils_calculate_directory_size_with_recursion.

@Barry-Xu-2018
Copy link
Contributor

@fujitatomoya

I have checked codes and LGTM.
So I not add comments.

Signed-off-by: Scott K Logan <[email protected]>

Co-authored-by: tomoya <[email protected]>
@cottsay cottsay merged commit 666241e into master Jan 15, 2021
@delete-merged-branch delete-merged-branch bot deleted the cottsay/dir_iter branch January 15, 2021 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants