You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@arrow.apache.org by "anjakefala (via GitHub)" <gi...@apache.org> on 2023/12/29 00:29:19 UTC

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

anjakefala opened a new issue, #39385:
URL: https://github.com/apache/arrow/issues/39385

   ### 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](https://man7.org/linux/man-pages/man2/rename.2.html) and [Posix standard](https://pubs.opengroup.org/onlinepubs/9699919799/functions/rename.html) do specify that a return value of -1 is expected for error calls, the [C++ reference](https://en.cppreference.com/w/cpp/io/c/rename) 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++


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@arrow.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [C++] Reviewing the `rename` call in `LocalFileSystem::Move`. [arrow]

Posted by "anjakefala (via GitHub)" <gi...@apache.org>.

anjakefala commented on issue #39385:
URL: https://github.com/apache/arrow/issues/39385#issuecomment-1879154171

   > 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::`! =))
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [C++] Reviewing the `rename` call in `LocalFileSystem::Move`. [arrow]

Posted by "anjakefala (via GitHub)" <gi...@apache.org>.

anjakefala commented on issue #39385:
URL: https://github.com/apache/arrow/issues/39385#issuecomment-1871644065

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [C++] Reviewing the `rename` call in `LocalFileSystem::Move`. [arrow]

Posted by "kou (via GitHub)" <gi...@apache.org>.

kou closed issue #39385: [C++] Reviewing the `rename` call in `LocalFileSystem::Move`.
URL: https://github.com/apache/arrow/issues/39385


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [C++] Reviewing the `rename` call in `LocalFileSystem::Move`. [arrow]

Posted by "felipecrv (via GitHub)" <gi...@apache.org>.

felipecrv commented on issue #39385:
URL: https://github.com/apache/arrow/issues/39385#issuecomment-1877249853

   `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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [C++] Reviewing the `rename` call in `LocalFileSystem::Move`. [arrow]

Posted by "mapleFU (via GitHub)" <gi...@apache.org>.

mapleFU commented on issue #39385:
URL: https://github.com/apache/arrow/issues/39385#issuecomment-1872511372

   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/ 
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [C++] Reviewing the `rename` call in `LocalFileSystem::Move`. [arrow]

Posted by "mapleFU (via GitHub)" <gi...@apache.org>.

mapleFU commented on issue #39385:
URL: https://github.com/apache/arrow/issues/39385#issuecomment-1871695672

   `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](https://man7.org/linux/man-pages/man3/errno.3.html) is set to indicate the error.
   
   So generally I think it's ok here


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [C++] Reviewing the `rename` call in `LocalFileSystem::Move`. [arrow]

Posted by "anjakefala (via GitHub)" <gi...@apache.org>.

anjakefala commented on issue #39385:
URL: https://github.com/apache/arrow/issues/39385#issuecomment-1872266024

   > 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](https://linux.die.net/man/2/fcntl) seems to contain file descriptor notifications?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org