[webkit-reviews] review granted: [Bug 225550] Port Filesystem::pathByAppendingComponent() & Filesystem:: pathByAppendingComponents() to std::filesystem : [Attachment 428072] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat May 8 09:50:57 PDT 2021

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 225550: Port Filesystem::pathByAppendingComponent() & Filesystem::
pathByAppendingComponents() to std::filesystem

Attachment 428072: Patch


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

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

> Source/WTF/wtf/FileSystem.cpp:740
> +    std::filesystem::path fsPath = fileSystemRepresentation(path).data();
> +    fsPath.append(fileSystemRepresentation(component).data());
> +    return String::fromUTF8(fsPath.u8string().c_str());

It’s a little strange to use fileSystemRepresentation to convert from a
WTF::String to a path, but then use String::fromUTF8 in the other direction.
Are we meant to be using an abstraction or just writing UTF-8 coding.

I don’t think writing String::fromUTF8(fsPath.u8string().c_str()) repeatedly is
a good pattern for the file system code. There should be a single helper
function to do this operation.

Similarly, to create a std::filesystem::path from a WTF::String we should use a
single helper function. We may later want to write a more efficient version of
it that doesn’t involve CString, and there’s no reason to sprinkle the use of
CString into all those functions. Same with append. There are iterator versions
that should be able to append things directly from transformed WTF::String
characters, and so we should not build the less efficient algorithm into each
call site. Instead we should concentrate these in a small number of conversion

> Source/WTF/wtf/FileSystem.cpp:745
> +    std::filesystem::path fsPath =

This makes me think that fileSystemRepresentation should use a StringView
instead of a String.

More information about the webkit-reviews mailing list