Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Refactor pstd/env with std::filesystem #1470

Merged
merged 1 commit into from
Jun 6, 2023
Merged

feat: Refactor pstd/env with std::filesystem #1470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions src/pika_stable_log.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,11 @@ bool StableLog::PurgeFiles(uint32_t to, bool manual) {
}

// Do delete
pstd::Status s = pstd::DeleteFile(log_path_ + it->second);
if (s.ok()) {
if (pstd::DeleteFile(log_path_ + it->second)) {
++delete_num;
--remain_expire_num;
} else {
LOG(WARNING) << log_path_ << " Purge log file : " << (it->second) << " failed! error:" << s.ToString();
LOG(WARNING) << log_path_ << " Purge log file : " << (it->second) << " failed! error: delete file failed";
}
} else {
// Break when face the first one not satisfied
Expand Down Expand Up @@ -209,11 +208,11 @@ Status StableLog::PurgeFileAfter(uint32_t filenum) {
for (auto& it : binlogs) {
if (it.first > filenum) {
// Do delete
Status s = pstd::DeleteFile(log_path_ + it.second);
if (!s.ok()) {
return s;
auto filename = log_path_ + it.second;
if (!pstd::DeleteFile(filename)) {
return Status::IOError("pstd::DeleteFile faield, filename = " + filename);
}
LOG(WARNING) << "Delete file " << log_path_ + it.second;
LOG(WARNING) << "Delete file " << filename;
}
}
return Status::OK();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch looks fine.
There is an improvement suggestion that can be made, where instead of concatenating strings in the DeleteFile function call, it will be better to pass it as a separate parameter.

So instead of:

pstd::DeleteFile(log_path_ + it->second)

It should be:

pstd::DeleteFile(log_path_, it->second)

This can help make the code more understandable and avoid any string concatenation issues.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch seems to be a review of a piece of code that deals with purging files. Here are a few points about the code:

  • There are no obvious bug risks in the code patch.
  • The improvement that could be suggested in this code patch is to log the actual reason for failure when deleting a file fails instead of using a generic error message. This can give more information on what went wrong during the delete operation and help debug the issue.
  • In the function PurgeFileAfter, it would be better to concatenate the filename outside of the pstd::DeleteFile call and store it in a variable to avoid code duplication, as done in the PurgeFiles function.
  • It may be good to consider adding more checks before deleting files, such as verifying file ownership (if applicable) and checking whether the file being deleted is actually the one intended to be deleted.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the code patch looks fine. However, there are a few improvements that could be made:

  1. In both functions, it would be better to use a range-based for loop instead of an iterator.

  2. In the PurgeFiles function, the if (s.ok()) check is unnecessary. The return value of pstd::DeleteFile is already a boolean indicating success or failure.

  3. In both functions, it would be better to store the full path of the file to be deleted in a separate variable before calling pstd::DeleteFile. This makes it easier to log in case of errors.

  4. In the error message of the PurgeFiles function, it would be better to provide more information about why the delete failed instead of just saying "delete file failed".

  5. In the PurgeFileAfter function, the misspelling of "failed" in the error message should be corrected.

  6. In both functions, it might be a good idea to add a check that the file exists before attempting to delete it, to avoid deleting non-existent files.

  7. Finally, it would be nice if there were comments explaining what the functions do and how they work.

Expand Down
19 changes: 13 additions & 6 deletions src/pstd/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,16 @@ target_include_directories(pstd
${INSTALL_INCLUDEDIR}
)

target_link_libraries(pstd
PUBLIC ${GLOG_LIBRARY}
${GFLAGS_LIBRARY}
${FMT_LIBRARY}
${LIBUNWIND_LIBRARY}
)
set(PSTD_DEP_LIBS ${GLOG_LIBRARY} ${GFLAGS_LIBRARY} ${FMT_LIBRARY} ${LIBUNWIND_LIBRARY})

if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
if(CMAKE_CXX_COMPILER_VERSION LESS 9)
list(APPEND PSTD_DEP_LIBS stdc++fs)
endif()
elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
if(CMAKE_CXX_COMPILER_VERSION LESS 9)
list(APPEND PSTD_DEP_LIBS c++fs)
endif()
endif()

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch seems to be adding support for linking the C++ filesystem library for GNU and Clang compilers based on their IDs. Here are some suggestions:

  1. It is a good practice to add comments in code that explain why certain changes are made. This can help other developers understand the reasoning behind the changes easily.

  2. The code patch only checks if the compiler is GNU or Clang, but it does not verify if the compiler supports the C++ filesystem library. You may want to consider adding an additional check for this using the check_cxx_compiler_flag function and setting a flag variable accordingly.

  3. When linking libraries, it is recommended to use target-based linkage instead of specifying libraries directly. This way, dependencies between targets can be automatically resolved by CMake. You can create a target with the add_library command and specify its dependencies using target_link_libraries. Then, link the target to your main target using target_link_libraries.

  4. It is better to use a switch statement instead of multiple if-elseif blocks when checking for different compiler IDs. This can make the code more concise and easier to read.

  5. Finally, you may want to consider using consistent casing for variable names and formatting the code consistently throughout the project.

target_link_libraries(pstd PUBLIC ${PSTD_DEP_LIBS})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code patch modifies the CMake project file for a target named "pstd". The patch adds a condition to link the standard C++ file system library "stdc++fs" on GNU compilers versions less than 9, and "c++fs" on Clang compilers versions less than 9. This is done by appending the required library to a list of dependent libraries called PSTD_DEP_LIBS. The patch also replaces the direct linking of dependent libraries with the target_link_libraries command, making it more concise.

A possible improvement to this patch is to use the CMAKE_CXX_STANDARD_LIBRARY_LIB variable instead of manually appending the required library for each compiler and version. The CMAKE_CXX_STANDARD_LIBRARY_LIB variable contains the name of the C++ standard library implementation (e.g., "stdc++", "c++") and can be used to link the corresponding file system library.

Another potential improvement is to add a comment explaining the reason for linking the file system library only on specific compiler versions. Without proper documentation, future maintainers might not understand why the library is linked conditionally, leading to confusion and potential bugs.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch seems to be modifying the CMake build script for the pstd target, specifically the linking of dependent libraries.

The changes appear to add conditional linker flags/arguments to link against the stdc++fs or c++fs library depending on the version of the GCC or Clang compiler.

There are no obvious bug risks in this code patch, but it is always good to test the changes and ensure they work as intended. As for improvement suggestions, it is not clear from this code patch if there are any other areas that could be improved without more context and knowledge about the project.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code patch seems to update the target_link_libraries command for the pstd target in a CMake project.

The changes look reasonable, as it defines an intermediate PSTD_DEP_LIBS variable to hold the dependent libraries and then links them with the pstd target using target_link_libraries.

The additional code checks for the compiler type and version. If the compiler is GCC or Clang and its version is less than 9, it appends the stdc++fs or c++fs library (depending on the compiler) to the PSTD_DEP_LIBS variable. This is because these libraries were introduced in GCC and Clang 9 respectively.

One potential improvement could be to use target_compile_features instead of checking the compiler versions manually. For example, you could use target_compile_features(pstd PUBLIC cxx_std_17) to enforce that the code is compiled with C++17 or later features, which should automatically include the filesystem library for GCC 9+ and Clang 9+.

7 changes: 2 additions & 5 deletions src/pstd/include/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ uint64_t Du(const std::string& filename);
*/
bool FileExists(const std::string& path);

Status DeleteFile(const std::string& fname);
bool DeleteFile(const std::string& fname);

int RenameFile(const std::string& oldname, const std::string& newname);

Expand All @@ -57,11 +57,8 @@ class FileLock : public pstd::noncopyable {
std::string name_;
};

Status LockFile(const std::string& f, FileLock** l);
Status UnlockFile(FileLock* l);

int GetChildren(const std::string& dir, std::vector<std::string>& result);
bool GetDescendant(const std::string& dir, std::vector<std::string>& result);
void GetDescendant(const std::string& dir, std::vector<std::string>& result);

uint64_t NowMicros();
void SleepForMicroseconds(int micros);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the provided diff, here are a few observations:

  • The return type of DeleteFile is changed from Status to bool. This may introduce some semantic changes and can affect error handling depending on how the API is used.
  • LockFile and UnlockFile functions are removed from the header file, so they are no longer part of the public API. This may be intentional or accidental, but it's worth checking if any downstream code depends on these functions.
  • The return type of GetDescendant is changed from bool to void, implying that the function no longer returns success/failure status. It's unclear if this was intentional.
  • Overall, it's hard to say anything more definitive without knowing what the implementation of these functions looks like, their purpose, and how they're used in the code.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch seems to change the return type of the DeleteFile function from Status to bool, and remove the declarations for LockFile and UnlockFile. It also changes the return type of GetDescendant from bool to void.

Without seeing the implementation of these functions or the context in which they are used, it is hard to determine whether these changes introduce any bugs or potential improvements. However, changing the return types could break compatibility with existing code that uses these functions, so it is important to make sure any other code using these functions is updated accordingly.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch looks like a simple header file with function signatures and declarations, so it's hard to provide any concrete feedback without seeing the implementation details. However, some general suggestions include:

  • Use consistent naming conventions for functions, such as using either camelCase or snake_case, and stick to them throughout the codebase.
  • Use descriptive function names that make clear what the function does, especially for public-facing functions.
  • Consider using std::filesystem instead of rolling your own file manipulation functions, as it provides a more robust and standardized API for dealing with files and directories.
  • The return type of the DeleteFile() function has been changed from Status to bool. Make sure this is intentional and doesn't introduce bugs or unexpected behavior elsewhere in the code.
  • Consider adding documentation comments (///) to the function signatures, describing their purpose and expected behavior.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch seems to be a header file defining various file-related functions and classes. Here are some observations:

  1. It would be better to return Status instead of bool in the DeleteFile function, as it allows conveying specific error messages to the caller.

  2. Similarly, LockFile and UnlockFile should also return Status instead of bool.

  3. The visibility of the GetDescendant function is unclear - it is not clear whether it is supposed to be used outside the header file or just within its implementation. If it is meant to be used outside, then it should have a status return value to indicate any errors.

  4. It would be good to add documentation (comments or doxygen-style) to provide context and usage examples for all these functions.

  5. It's hard to give specific improvement suggestions without knowing the intended use of these functions. For example, it might make sense to add overloads for DeleteFile and LockFile that take std::filesystem::path objects instead of a string.

Expand Down
Loading