[webkit-reviews] review granted: [Bug 225633] Port WTF::FileSystem::listDirectory to std::filesystem : [Attachment 428241] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 225633: Port WTF::FileSystem::listDirectory to std::filesystem
https://bugs.webkit.org/show_bug.cgi?id=225633

Attachment 428241: Patch

https://bugs.webkit.org/attachment.cgi?id=428241&action=review




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 428241
  --> https://bugs.webkit.org/attachment.cgi?id=428241
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428241&action=review

> Source/WTF/ChangeLog:16
> +	   Using a regular expression has the benefit of having an
cross-platform implementation available
> +	   in the STL.

We also have a regular expression library in JavaScriptCore. Do we really want
to start using std::regex, even for this?

> Source/WTF/wtf/FileSystem.h:141
> +WTF_EXPORT_PRIVATE Vector<String> listDirectoryUsingRegex(const String&
path, const String& regex);

I’m not convinced we need a version of this that takes const String&. I would
think we could just take const char* for the regular expression.

Regular expression seems overkill for this. Do we really need this function of
filtering by regular expression built into the function? Can’t callers just
skip things they want to ignore? Most clients are just checking for a prefix or
suffix, although a few are doing something fancier.

> Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:94
> -    return makeString(constructedPathPrefix(legacyFilename), '*');
> +    return makeString(constructedPathPrefix(legacyFilename), ".*");

Concatenating two constant strings can be done at compile time, and getting the
length of a constant string can also be done at compile time. There is no need
to involve WTF::String in this code.

It’s not safe to just take a string from a function result and use it as a
regular expression. This is safe only because constructedPathPrefix doesn’t
return an arbitrary string, but rather just one of two fixed string constants
that are guaranteed not to have any special characters.

> Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:505
> +	   Vector<WTF::String> fullPaths = listDirectoryUsingRegex(storePath,
constructedPathFilter(false));
> +	   Vector<WTF::String> legacyFullPaths =
listDirectoryUsingRegex(storePath, constructedPathFilter(true));

auto

> Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:507
>	   identifiers.reserveInitialCapacity(fullPaths.size() +
legacyFullPaths.size());

This path code below is using reverseFind('/') but std::filesystem has
functions for doing this kind of operation without that.