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 filtering #106

Merged
merged 2 commits into from
Sep 12, 2024
Merged

Conversation

ckaiser
Copy link
Collaborator

@ckaiser ckaiser commented Sep 7, 2024

As part of my work on #4610 I've added directory/file filtering and pattern matching to base::list_files and made some optimizations and other changes:

  • Now avoids allocations as much as possible
  • Uses FindFirstFileEx on Windows which has flags to get directories directly and returns a more basic structure.
  • Uses fnmatch on Unix to replicate the wildcard matching Windows behavior

This allows us to use list_files much more efficiently, since before you had to do:

for (auto& item : base::list_files(path)) {
  if (base::is_file(base::join_path(path, item)) && base::get_file_extension(item) == "png")
	do_something(item);
 }

which caused file attributes to be fetched twice, once when listing and another time when calling is_file, resulting in double the amount of IO operations. Now we can now just do:

for (auto& item : base::list_files(path, ItemType::Files, "*.png")) {
	do_something(item);
 }

which is faster, and IMO more ergonomic.

I'll be working on a PR to modify aseprite to take advantage of this next, figured we can be getting the benefits of what I'm doing for the "recent files" fix and it'll make that easier to review.

There's a couple of things for which I'd like your opinion @dacap:

  • "list_files" as a name is technically correct on Linux since "everything is a file" but in practical terms the function is a misnomer, do we wanna change that? Leaving it is fine it's just a pet peeve of mine.
  • Speaking of names, I struggled naming the filtering enum, I don't like the word "type" in there, taking suggestions.
  • This does not break source compatibility but definitely breaks binary compatibility, is that something we care about at this point in time?

Copy link
Collaborator

@aseprite-bot aseprite-bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

base/fs_tests.cpp Outdated Show resolved Hide resolved
base/fs_tests.cpp Outdated Show resolved Hide resolved
base/fs_tests.cpp Outdated Show resolved Hide resolved
base/fs_tests.cpp Outdated Show resolved Hide resolved
base/fs_tests.cpp Outdated Show resolved Hide resolved
base/fs_tests.cpp Outdated Show resolved Hide resolved
base/fs_unix.h Outdated Show resolved Hide resolved
base/fs_unix.h Outdated Show resolved Hide resolved
base/fs_unix.h Show resolved Hide resolved
base/fs_win32.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@aseprite-bot aseprite-bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -229,20 +230,32 @@ std::string get_absolute_path(const std::string& path)
return full;
}

paths list_files(const std::string& path)
paths list_files(const std::string& path, ItemType filter, const std::string& match)
Copy link
Collaborator

Choose a reason for hiding this comment

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

warning: function 'list_files' defined in a header file; function definitions in header files can lead to ODR violations [misc-definitions-in-headers]

paths list_files(const std::string& path, ItemType filter, const std::string& match)
      ^
Additional context

base/fs_unix.h:232: make as 'inline'

paths list_files(const std::string& path, ItemType filter, const std::string& match)
      ^

@ckaiser ckaiser assigned ckaiser and dacap and unassigned ckaiser Sep 10, 2024
Copy link
Member

@dacap dacap left a comment

Choose a reason for hiding this comment

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

LGTM! Just minor changes about indentation (probably when the const keyword was removed).

Comment on lines +68 to +73
// Item types that list_files() can be filtered by
enum class ItemType {
All,
Directories,
Files
};
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if using flags might simplify the code (minor change: fixed the indentation):

Suggested change
// Item types that list_files() can be filtered by
enum class ItemType {
All,
Directories,
Files
};
// Item types that list_files() can be filtered by
enum class ItemType {
Directories = 1,
Files = 2,
All = 3
};
LAF_ENUM_FLAGS(ItemType);

The LAF_ENUM_FLAGS macro is something new to make flags easier to use for enum class-type (you will need to rebase to the latest main to use this macro if you wish).

Anyway this change is not needed, we can use the ItemType as you've defined it, just fixing the indentation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I'll keep a simple enum class since you can't really mix and match flags in this case, if you want Directories | Files you use "All", and if you want Directories | All... you... well you shouldn't want that, that makes no sense 🤪

base/fs.h Outdated Show resolved Hide resolved
base/fs_win32.h Outdated Show resolved Hide resolved
base/fs_win32.h Outdated Show resolved Hide resolved
@dacap dacap assigned ckaiser and unassigned dacap Sep 11, 2024
@ckaiser ckaiser assigned dacap and unassigned ckaiser Sep 12, 2024
@dacap dacap merged commit 608e099 into aseprite:main Sep 12, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants