[webkit-reviews] review granted: [Bug 225255] Start leveraging std::filesystem in WTF::FileSystem : [Attachment 427476] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 225255: Start leveraging std::filesystem in WTF::FileSystem
https://bugs.webkit.org/show_bug.cgi?id=225255

Attachment 427476: Patch

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




--- Comment #10 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 427476
  --> https://bugs.webkit.org/attachment.cgi?id=427476
Patch

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

> Source/WTF/ChangeLog:9
> +	   Start leveraging std::filesystem in WTF::FileSystem to reduce the
amount of
> +	   platform-specific code.

I think doing this in WTF::FileSystem at first is a good idea, but I would also
like to eventually see WTF::FileSystem removed in favor of direct usage of
std::filesystem if we can. Time will tell.

> Source/WTF/wtf/FileSystem.cpp:52
> +namespace fs = std::filesystem;

Not a huge fan of this. I think having longer names is probably ok, but I also
think this is acceptable in a cpp file.

> Source/WTF/wtf/FileSystem.cpp:491
> +    return fs::exists(fileSystemRepresentation(path).data(), ec);

Would be good to explain why the error code is not being checked, since it
seems odd at first glance.

> Source/WTF/wtf/FileSystem.cpp:501
> +#if !PLATFORM(MAC)

Would be good to have a comment explaining why macOS has a non-default
implementation.