Bug 225550 – Port Filesystem::pathByAppendingComponent() & Filesystem:: pathByAppendingComponents() to std::filesystem

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Chris Dumez]

Port Filesystem::pathByAppendingComponent() & Filesystem:: pathByAppendingComponents() to std::filesystem.

[Chris Dumez]

Created attachment 428062 [details]
Patch

[Chris Dumez]

Created attachment 428072 [details]
Patch

[Darin Adler]

Comment on attachment 428072 [details]
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.

[Chris Dumez]

Created attachment 428088 [details]
Patch

[Chris Dumez]

(In reply to Darin Adler from comment #3)
> Comment on attachment 428072 [details]
> 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.

I added 2 utility functions to convert between String <-> std::filesystem::path. Both rely on UTF-8 and the one that converts to a std::filesystem::path takes in a StringView.

[Darin Adler]

(In reply to Chris Dumez from comment #5)
> I added 2 utility functions to convert between String <->
> std::filesystem::path. Both rely on UTF-8 and the one that converts to a
> std::filesystem::path takes in a StringView.

With these, you should be able to use auto a bit more, and perhaps cut down on the number of local variables, since we’ll get std::filesystem::path without having to utter that type name explicitly.

[Chris Dumez]

(In reply to Darin Adler from comment #6)
> (In reply to Chris Dumez from comment #5)
> > I added 2 utility functions to convert between String <->
> > std::filesystem::path. Both rely on UTF-8 and the one that converts to a
> > std::filesystem::path takes in a StringView.
> 
> With these, you should be able to use auto a bit more, and perhaps cut down
> on the number of local variables, since we’ll get std::filesystem::path
> without having to utter that type name explicitly.

I did switch to auto in my patch. It is true that I could likely reduce the number of locals too. I will check.

[Chris Dumez]

Created attachment 428089 [details]
Patch

[EWS]

Committed r277231 (237500@main): <https://commits.webkit.org/237500@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428089 [details].

[Radar WebKit Bug Importer]

<rdar://problem/77701228>

[Darin Adler]

Comment on attachment 428089 [details]
Patch

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

> Source/WTF/wtf/FileSystem.cpp:55
> +    return std::filesystem::u8path(path.utf8().data());

Did you research what is different about fileSystemRepresentation than just converting to UTF-8? Just want to be sure we didn’t lose anything valuable when we dropped it.

[Chris Dumez]

(In reply to Darin Adler from comment #11)
> Comment on attachment 428089 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428089&action=review
> 
> > Source/WTF/wtf/FileSystem.cpp:55
> > +    return std::filesystem::u8path(path.utf8().data());
> 
> Did you research what is different about fileSystemRepresentation than just
> converting to UTF-8? Just want to be sure we didn’t lose anything valuable
> when we dropped it.

I will double check but u8path() will take care of converting from Utf-8 to the native filesystem encoding for us, which means we should not need fileSystemRepresentation() anymore.