Move file operations to <filesystem> where possible #113
Conversation
matthewtff
force-pushed
the
move-to-filesystem
branch
from
78bcdd8 to
14bbdf8
Compare
src/base/file/file_posix.cc
Outdated
| } | ||
|
|
||
| return buffer.st_size; | ||
| return File::Size(path_, error); |
There was a problem hiding this comment.
This one may be a bad idea, as we can't track directory changes and path could be a relative one =/
There was a problem hiding this comment.
It also doesn't reuse opened descriptor.
| @@ -16,6 +16,7 @@ namespace base { | |||
|
|
|||
| class Literal { | |||
| public: | |||
| inline const char* c_str() const { return str_; } | |||
There was a problem hiding this comment.
In ReplaceTildeInPath from file_cache.cc.
GetHomeDir() returns a Literal thats why I should use c_str() to create a Path form it.
| @@ -52,5 +59,19 @@ inline void GetLastError(String* error) { | |||
| } | |||
| } | |||
|
|
|||
| inline bool SetPermissions(const Path& path, const Perms permissions, | |||
There was a problem hiding this comment.
It should fit the machine word, but I'll make it a const ref no difference for me.
src/base/file/file_posix.cc
Outdated
| return false; | ||
| } | ||
|
|
||
| return (buffer.st_mode & S_IFDIR) == 0; | ||
| return std::experimental::filesystem::is_regular_file(status); |
There was a problem hiding this comment.
It's not equivalent replacement - now we can't work with pipes, sockets, char or block devices.
There was a problem hiding this comment.
Ok, I'll change it to !is_directory
src/base/file/file_posix.cc
Outdated
|
|
||
| return true; | ||
| return std::experimental::filesystem::exists(status) && | ||
| std::experimental::filesystem::is_regular_file(status); |
There was a problem hiding this comment.
is_regular_file() here is not equivalent too.
src/base/file/file_posix.cc
Outdated
| } | ||
|
|
||
| return buffer.st_size; | ||
| return File::Size(path_, error); |
There was a problem hiding this comment.
It also doesn't reuse opened descriptor.
src/base/file/file_test.cc
Outdated
|
|
||
| namespace dist_clang { | ||
| namespace base { | ||
|
|
||
| TEST(FileTest, Read) { | ||
| const auto expected_content = "All your base are belong to us"_l; | ||
| const base::TemporaryDir temp_dir; | ||
| const String file_path = String(temp_dir) + "/file"; | ||
| const Path temp_dir_path = temp_dir.GetPath(); |
There was a problem hiding this comment.
Why not simply add operator Path? Then it will look like: Path(tmp_dir) / …
There was a problem hiding this comment.
As tmp_dir.GetPath() looks more explicit than operator Path().
But that's matter of taste, so let get the operator way...
| namespace dist_clang { | ||
| namespace base { | ||
|
|
||
| void WalkDirectory( |
There was a problem hiding this comment.
This is a hard part - I'll review this function later.
|
|
||
| bool RemoveEmptyDirectory(const Path& path) { | ||
| std::error_code ec; | ||
| return std::experimental::filesystem::remove(path, ec); |
There was a problem hiding this comment.
We can accidentally pass the file and remove it!
There was a problem hiding this comment.
Is it OK to add a std::fs::is_directory check, or you would like to leave previous code for performance reasons?
src/base/file_utils.cc
Outdated
|
|
||
| bool CreateDirectory(const Path& path, String* error) { | ||
| std::error_code ec; | ||
| std::experimental::filesystem::create_directories(path, ec); |
There was a problem hiding this comment.
It sets 0777 perms by default, while we want 0755
There was a problem hiding this comment.
Looks like there's no way to implement this using :(
I'll revert this change, thanks.
src/base/file_utils_test.cc
Outdated
|
|
||
| ASSERT_TRUE(CreateDirectory(temp, &error)) << error; | ||
| ASSERT_TRUE(CreateDirectory(temp.string(), &error)) << error; |
|
Should I close this one now? |
|
No, I want to use this PR as a reference for a further work. |
No description provided.