Bug 225550 - Port Filesystem::pathByAppendingComponent() & Filesystem:: pathByAppendingComponents() to std::filesystem
Summary: Port Filesystem::pathByAppendingComponent() & Filesystem:: pathByAppendingCom...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 225524
Blocks:
  Show dependency treegraph
 
Reported: 2021-05-07 16:44 PDT by Chris Dumez
Modified: 2021-05-08 18:27 PDT (History)
12 users (show)

See Also:


Attachments
Patch (15.51 KB, patch)
2021-05-07 16:47 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (15.90 KB, patch)
2021-05-07 19:14 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (21.91 KB, patch)
2021-05-08 10:30 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (21.81 KB, patch)
2021-05-08 12:14 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-05-07 16:44:30 PDT
Port Filesystem::pathByAppendingComponent() & Filesystem:: pathByAppendingComponents() to std::filesystem.
Comment 1 Chris Dumez 2021-05-07 16:47:53 PDT
Created attachment 428062 [details]
Patch
Comment 2 Chris Dumez 2021-05-07 19:14:33 PDT
Created attachment 428072 [details]
Patch
Comment 3 Darin Adler 2021-05-08 09:50:57 PDT
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.
Comment 4 Chris Dumez 2021-05-08 10:30:58 PDT
Created attachment 428088 [details]
Patch
Comment 5 Chris Dumez 2021-05-08 10:33:19 PDT
(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.
Comment 6 Darin Adler 2021-05-08 11:06:00 PDT
(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.
Comment 7 Chris Dumez 2021-05-08 11:27:52 PDT
(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.
Comment 8 Chris Dumez 2021-05-08 12:14:46 PDT
Created attachment 428089 [details]
Patch
Comment 9 EWS 2021-05-08 15:35:15 PDT
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].
Comment 10 Radar WebKit Bug Importer 2021-05-08 15:36:28 PDT
<rdar://problem/77701228>
Comment 11 Darin Adler 2021-05-08 16:26:36 PDT
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.
Comment 12 Chris Dumez 2021-05-08 18:27:19 PDT
(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.