[Support][FileSystem] Prefer status with file_t parameter. NFC.

[cachemeifyoucan]

When searching the documentation for LLVM for llvm::sys::fs::status,
only the version with file descriptor can be found because the
documentation is built on the UNIX host and the file handle version is
defined out.

In reality, file handle version is more portable and should be displayed
instead. On UNIX, these two functions have the same underlying type,
thus it shouldn't matter which one is declared, and both versions are
still available on Windows for compatibility.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Steven Wu (cachemeifyoucan)

Changes

When searching the documentation for LLVM for llvm::sys::fs::status,
only the version with file descriptor can be found because the
documentation is built on the UNIX host and the file handle version is
defined out.

In reality, file handle version is more portable and should be displayed
instead. On UNIX, these two functions have the same underlying type,
thus it shouldn't matter which one is declared, and both versions are
still available on Windows for compatibility.

---

Full diff: https://github.com/llvm/llvm-project/pull/157581.diff

1 Files Affected:

• (modified) llvm/include/llvm/Support/FileSystem.h (+3-3)

diff --git a/llvm/include/llvm/Support/FileSystem.h b/llvm/include/llvm/Support/FileSystem.h
index a21b0a272d2b0..88299cab7d7ee 100644
--- a/llvm/include/llvm/Support/FileSystem.h
+++ b/llvm/include/llvm/Support/FileSystem.h
@@ -640,12 +640,12 @@ LLVM_ABI std::error_code is_other(const Twine &path, bool &result);
 LLVM_ABI std::error_code status(const Twine &path, file_status &result,
                                 bool follow = true);
 
-/// A version for when a file descriptor is already available.
-LLVM_ABI std::error_code status(int FD, file_status &Result);
+/// A version for when a file handle is already available.
+LLVM_ABI std::error_code status(file_t FD, file_status &Result);
 
 #ifdef _WIN32
 /// A version for when a file descriptor is already available.
-LLVM_ABI std::error_code status(file_t FD, file_status &Result);
+LLVM_ABI std::error_code status(int FD, file_status &Result);
 #endif
 
 /// Get file creation mode mask of the process.

[cachemeifyoucan]

Possible followup:

• Following APIs still only have the FD version: tryLockFile, lockFile, unlockFile. Create file_t versions of them.
• Maybe deprecating the FD versions of the functions in llvm::sys::fs

This makes no need to call convertFDToNativeFile and makes code more portable. Let me know if these are not reasonable.

[compnerd]

Preferring the handle version of the API is reasonable, but if this is purely for documentation purposes, I think that we should change the guard instead of changing which variant is under the guard. #if defined(_WIN32) || defined(DOXYGEN) should allow indexing. The original version is clearer in intention: the FD version is "universal" since everything on Unix has a file descriptor. Windows almost always would prefer the file handle variant, and so with this change, the code is more confusing, why does windows only have the file descriptor version?

[cachemeifyoucan]

Preferring the handle version of the API is reasonable, but if this is purely for documentation purposes, I think that we should change the guard instead of changing which variant is under the guard. #if defined(_WIN32) || defined(DOXYGEN) should allow indexing. The original version is clearer in intention: the FD version is "universal" since everything on Unix has a file descriptor. Windows almost always would prefer the file handle variant, and so with this change, the code is more confusing, why does windows only have the file descriptor version?

It is actually only windows has the explicit file descriptor version. Everywhere else is file handle version because it is just the same type (I hope that won't introduce ambiguity in the code).

[compnerd]

Oh, that is a rather annoying ambiguity. file_t is a typedef, and if the enclosing namespace is pulled in, then we cannot differentiate. I suppose that this is reasonable for addressing the documentation issue. Could you please leave a comment about this in the code as it is very confusing to see only Windows having the file descriptor overload?

[cachemeifyoucan]

The other reason for the change is if we can find a way to seamlessly switching to file_t when referring to file descriptor/handle. See my first comment above.

[cachemeifyoucan]

Related, write an RFC here: https://discourse.llvm.org/t/rfc-unified-llvm-fs-apis-to-use-sys-file-t-instead-of-int-file-descriptor/88240