[webkit-reviews] review granted: [Bug 225550] Port Filesystem::pathByAppendingComponent() & Filesystem:: pathByAppendingComponents() to std::filesystem : [Attachment 428072] 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 225550: Port Filesystem::pathByAppendingComponent() & Filesystem::
pathByAppendingComponents() to std::filesystem
https://bugs.webkit.org/show_bug.cgi?id=225550

Attachment 428072: Patch

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




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

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

> Source/WTF/wtf/FileSystem.cpp:745
> +    std::filesystem::path fsPath =
fileSystemRepresentation(path.toStringWithoutCopying()).data();

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