[C++] Reviewing the rename call in LocalFileSystem::Move.

[anjakefala]

Describe the enhancement requested

LocalFileSystem::Move on non-Windows, calls rename: https://github.com/apache/arrow/blob/main/cpp/src/arrow/filesystem/localfs.cc#L585. It then does a check for -1, and prints the error.

While the rename system call and Posix standard do specify that a return value of -1 is expected for error calls, the C++ reference specifies that a "non-zero" is returned upon error.

I have two questions:

1. Is the rename that is being called in LocalFileSystem::Move the one in std::? Should it be prefaced with std::?
2. Should the check be changed to a non-zero check, as opposed to a -1 check? Is there is a possibility that std::rename returns a different number other than -1 upon error?

Component(s)

C++

[anjakefala]

For reference, the LocalFileSystem::Move tests are here: https://github.com/apache/arrow/blob/main/cpp/src/arrow/filesystem/filesystem_test.cc#L701

[mapleFU]

rename(2) ( https://man7.org/linux/man-pages/man2/rename.2.html ) is a Posix api defined in here ( https://github.com/apache/arrow/blob/main/cpp/src/arrow/filesystem/localfs.cc#L28 ). We also having a std::filesystem in C++17 ( https://en.cppreference.com/w/cpp/filesystem/rename )

On success, zero is returned. On error, -1 is returned, and errno is set to indicate the error.

So generally I think it's ok here

[anjakefala]

rename(2) ( https://man7.org/linux/man-pages/man2/rename.2.html ) is a Posix api defined in here ( https://github.com/apache/arrow/blob/main/cpp/src/arrow/filesystem/localfs.cc#L28 ).

Out of curiousity, how did you figure out it was defined in fcntl.h? fcntl.h seems to contain file descriptor notifications?

[mapleFU]

Oh it might in stdio.h, I think rename here should be an POSIX api, so I search it in https://man7.org/linux/man-pages/

After find the api, we can know where we should include

[felipecrv]

libc++ simply re-exports the global rename defined in stdio.h to the std:: namespace [1].

I think the C++ standard, when defining rename didn't want to guarantee -1 is always returned even though the actual implementation guarantees that.

I wouldn't oppose changing the code from == -1 to < 0, but since we are using #ifdef _WIN32 and not trusting the rename provided by Windows via its cstdio, it's more idiomatic to keep the rename without the std:: for POSIX systems. This means we are relying on POSIX and guarding against C++ changing std::rename in the future (even though the chances of that happening are basically 0).

[1] https://github.com/llvm/llvm-project/blob/main/libcxx/include/cstdio#L157

[anjakefala]

it's more idiomatic to keep the rename without the std:: for POSIX systems. This means we are relying on POSIX and guarding against C++ changing std::rename in the future

Oh, thank you for explaining why it does not have the std::! =))