[llvm][Support] Make sys::fs::file_t into a seperate type #160588
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using spr 1.3.7
|
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-llvm-support Author: Steven Wu (cachemeifyoucan) ChangesInstead of using Full diff: https://github.com/llvm/llvm-project/pull/160588.diff 10 Files Affected:
diff --git a/llvm/include/llvm/Support/File.h b/llvm/include/llvm/Support/File.h
new file mode 100644
index 0000000000000..d1ce246cdb554
--- /dev/null
+++ b/llvm/include/llvm/Support/File.h
@@ -0,0 +1,55 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+/// \file
+/// This file declares llvm::sys::fs::file_t type.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_SUPPORT_FILE_H
+#define LLVM_SUPPORT_FILE_H
+
+namespace llvm::sys::fs {
+
+/// This class wraps the platform specific file handle/descriptor type to
+/// provide an unified representation.
+struct file_t {
+#if defined(_WIN32)
+ // A Win32 HANDLE is a typedef of void*
+ using value_type = void *;
+ static const value_type Invalid;
+#else
+ // A file descriptor on UNIX.
+ using value_type = int;
+ static constexpr value_type Invalid = -1;
+#endif
+ value_type Value;
+
+ /// Default constructor to invalid file.
+ file_t() : Value(Invalid) {}
+
+ /// Implicit constructor from underlying value.
+ // TODO: Make this explicit to flush out type mismatches.
+ file_t(value_type Value) : Value(Value) {}
+
+ /// Is a valid file.
+ bool isValid() const { return Value != Invalid; }
+
+ /// Get the underlying value and return a platform specific value.
+ value_type get() const { return Value; }
+};
+
+inline bool operator==(file_t LHS, file_t RHS) {
+ return LHS.get() == RHS.get();
+}
+
+inline bool operator!=(file_t LHS, file_t RHS) { return !(LHS == RHS); }
+
+} // namespace llvm::sys::fs
+
+#endif
diff --git a/llvm/include/llvm/Support/FileSystem.h b/llvm/include/llvm/Support/FileSystem.h
index c203779307840..72afcc4051854 100644
--- a/llvm/include/llvm/Support/FileSystem.h
+++ b/llvm/include/llvm/Support/FileSystem.h
@@ -35,6 +35,7 @@
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/File.h"
#include "llvm/Support/FileSystem/UniqueID.h"
#include "llvm/Support/MD5.h"
#include <cassert>
@@ -49,15 +50,6 @@ namespace llvm {
namespace sys {
namespace fs {
-#if defined(_WIN32)
-// A Win32 HANDLE is a typedef of void*
-using file_t = void *;
-#else
-using file_t = int;
-#endif
-
-LLVM_ABI extern const file_t kInvalidFile;
-
/// An enumeration for the file system's view of the type.
enum class file_type {
status_error,
@@ -645,13 +637,11 @@ 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 is already available.
+LLVM_ABI std::error_code status(file_t F, 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);
-#endif
+LLVM_ABI std::error_code status(int FD, file_status &Result);
/// Get file creation mode mask of the process.
///
@@ -1000,15 +990,15 @@ LLVM_ABI Expected<file_t> openNativeFile(const Twine &Name,
LLVM_ABI file_t convertFDToNativeFile(int FD);
#ifndef _WIN32
-inline file_t convertFDToNativeFile(int FD) { return FD; }
+inline file_t convertFDToNativeFile(int FD) { return file_t(FD); }
#endif
/// Return an open handle to standard in. On Unix, this is typically FD 0.
-/// Returns kInvalidFile when the stream is closed.
+/// Returns Invalid file_t when the stream is closed.
LLVM_ABI file_t getStdinHandle();
/// Return an open handle to standard out. On Unix, this is typically FD 1.
-/// Returns kInvalidFile when the stream is closed.
+/// Returns Invalid file_t when the stream is closed.
LLVM_ABI file_t getStdoutHandle();
/// Return an open handle to standard error. On Unix, this is typically FD 2.
diff --git a/llvm/include/llvm/Support/MemoryBuffer.h b/llvm/include/llvm/Support/MemoryBuffer.h
index f092c67265a31..e7f2b2090f453 100644
--- a/llvm/include/llvm/Support/MemoryBuffer.h
+++ b/llvm/include/llvm/Support/MemoryBuffer.h
@@ -21,23 +21,13 @@
#include "llvm/Support/CBindingWrapping.h"
#include "llvm/Support/Compiler.h"
#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/File.h"
#include "llvm/Support/MemoryBufferRef.h"
#include <cstddef>
#include <cstdint>
#include <memory>
namespace llvm {
-namespace sys {
-namespace fs {
-// Duplicated from FileSystem.h to avoid a dependency.
-#if defined(_WIN32)
-// A Win32 HANDLE is a typedef of void*
-using file_t = void *;
-#else
-using file_t = int;
-#endif
-} // namespace fs
-} // namespace sys
/// This interface provides simple read-only access to a block of memory, and
/// provides simple methods for reading files and standard input into a memory
diff --git a/llvm/lib/CAS/MappedFileRegionArena.cpp b/llvm/lib/CAS/MappedFileRegionArena.cpp
index 472843d78af6e..f5e76056a0698 100644
--- a/llvm/lib/CAS/MappedFileRegionArena.cpp
+++ b/llvm/lib/CAS/MappedFileRegionArena.cpp
@@ -373,7 +373,7 @@ Expected<int64_t> MappedFileRegionArena::allocateOffset(uint64_t AllocSize) {
ErrorOr<FileSizeInfo> FileSizeInfo::get(sys::fs::file_t File) {
#if LLVM_ON_UNIX && defined(MAPPED_FILE_BSIZE)
struct stat Status;
- int StatRet = ::fstat(File, &Status);
+ int StatRet = ::fstat(File.get(), &Status);
if (StatRet)
return errnoAsErrorCode();
uint64_t AllocatedSize = uint64_t(Status.st_blksize) * MAPPED_FILE_BSIZE;
diff --git a/llvm/lib/Object/ArchiveWriter.cpp b/llvm/lib/Object/ArchiveWriter.cpp
index 6fc0889afc6a8..efaec82f9b745 100644
--- a/llvm/lib/Object/ArchiveWriter.cpp
+++ b/llvm/lib/Object/ArchiveWriter.cpp
@@ -132,7 +132,7 @@ Expected<NewArchiveMember> NewArchiveMember::getFile(StringRef FileName,
if (!FDOrErr)
return FDOrErr.takeError();
sys::fs::file_t FD = *FDOrErr;
- assert(FD != sys::fs::kInvalidFile);
+ assert(FD.isValid());
if (auto EC = sys::fs::status(FD, Status))
return errorCodeToError(EC);
diff --git a/llvm/lib/Support/FileUtilities.cpp b/llvm/lib/Support/FileUtilities.cpp
index dbd6c324cf4dc..b8f92ac763e75 100644
--- a/llvm/lib/Support/FileUtilities.cpp
+++ b/llvm/lib/Support/FileUtilities.cpp
@@ -306,7 +306,8 @@ Error FilePermissionsApplier::apply(
return createFileError(OutputFilename, EC);
sys::fs::file_status OStat;
- if (std::error_code EC = sys::fs::status(FD, OStat))
+ if (std::error_code EC =
+ sys::fs::status(sys::fs::convertFDToNativeFile(FD), OStat))
return createFileError(OutputFilename, EC);
if (OStat.type() == sys::fs::file_type::regular_file) {
#ifndef _WIN32
diff --git a/llvm/lib/Support/Unix/Path.inc b/llvm/lib/Support/Unix/Path.inc
index 0d991ead72416..56b998d0d2440 100644
--- a/llvm/lib/Support/Unix/Path.inc
+++ b/llvm/lib/Support/Unix/Path.inc
@@ -120,8 +120,6 @@ namespace llvm {
namespace sys {
namespace fs {
-const file_t kInvalidFile = -1;
-
#if defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) || \
defined(__FreeBSD_kernel__) || defined(__linux__) || \
defined(__CYGWIN__) || defined(__DragonFly__) || defined(_AIX) || \
@@ -768,6 +766,10 @@ std::error_code status(const Twine &Path, file_status &Result, bool Follow) {
return fillStatus(StatRet, Status, Result);
}
+std::error_code status(file_t F, file_status &Result) {
+ return status(F.get(), Result);
+}
+
std::error_code status(int FD, file_status &Result) {
struct stat Status;
int StatRet = ::fstat(FD, &Status);
@@ -832,7 +834,7 @@ std::error_code setLastAccessAndModificationTime(int FD, TimePoint<> AccessTime,
#endif
}
-std::error_code mapped_file_region::init(int FD, uint64_t Offset,
+std::error_code mapped_file_region::init(file_t FD, uint64_t Offset,
mapmode Mode) {
assert(Size != 0);
@@ -861,13 +863,13 @@ std::error_code mapped_file_region::init(int FD, uint64_t Offset,
}
#endif // #if defined (__APPLE__)
- Mapping = ::mmap(nullptr, Size, prot, flags, FD, Offset);
+ Mapping = ::mmap(nullptr, Size, prot, flags, FD.get(), Offset);
if (Mapping == MAP_FAILED)
return errnoAsErrorCode();
return std::error_code();
}
-mapped_file_region::mapped_file_region(int fd, mapmode mode, size_t length,
+mapped_file_region::mapped_file_region(file_t fd, mapmode mode, size_t length,
uint64_t offset, std::error_code &ec)
: Size(length), Mode(mode) {
(void)Mode;
@@ -1128,9 +1130,9 @@ std::error_code openFile(const Twine &Name, int &ResultFD,
return std::error_code();
}
-Expected<int> openNativeFile(const Twine &Name, CreationDisposition Disp,
- FileAccess Access, OpenFlags Flags,
- unsigned Mode) {
+Expected<file_t> openNativeFile(const Twine &Name, CreationDisposition Disp,
+ FileAccess Access, OpenFlags Flags,
+ unsigned Mode) {
int FD;
std::error_code EC = openFile(Name, FD, Disp, Access, Flags, Mode);
@@ -1183,7 +1185,7 @@ std::error_code openFileForRead(const Twine &Name, int &ResultFD,
Expected<file_t> openNativeFileForRead(const Twine &Name, OpenFlags Flags,
SmallVectorImpl<char> *RealPath) {
- file_t ResultFD;
+ int ResultFD;
std::error_code EC = openFileForRead(Name, ResultFD, Flags, RealPath);
if (EC)
return errorCodeToError(EC);
@@ -1200,7 +1202,7 @@ Expected<size_t> readNativeFile(file_t FD, MutableArrayRef<char> Buf) {
#else
size_t Size = Buf.size();
#endif
- ssize_t NumRead = sys::RetryAfterSignal(-1, ::read, FD, Buf.data(), Size);
+ ssize_t NumRead = sys::RetryAfterSignal(-1, ::read, FD.get(), Buf.data(), Size);
if (NumRead == -1)
return errorCodeToError(errnoAsErrorCode());
// The underlying operation on these platforms allow opening directories
@@ -1224,7 +1226,7 @@ Expected<size_t> readNativeFileSlice(file_t FD, MutableArrayRef<char> Buf,
#endif
#ifdef HAVE_PREAD
ssize_t NumRead =
- sys::RetryAfterSignal(-1, ::pread, FD, Buf.data(), Size, Offset);
+ sys::RetryAfterSignal(-1, ::pread, FD.get(), Buf.data(), Size, Offset);
#else
if (lseek(FD, Offset, SEEK_SET) == -1)
return errorCodeToError(errnoAsErrorCode());
@@ -1297,8 +1299,8 @@ std::error_code unlockFile(int FD) {
std::error_code closeFile(file_t &F) {
file_t TmpF = F;
- F = kInvalidFile;
- return Process::SafelyCloseFileDescriptor(TmpF);
+ F = file_t::Invalid;
+ return Process::SafelyCloseFileDescriptor(TmpF.get());
}
template <typename T>
diff --git a/llvm/lib/Support/VirtualFileSystem.cpp b/llvm/lib/Support/VirtualFileSystem.cpp
index cf784595c2f1c..e71a5b691f6c5 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/llvm/lib/Support/VirtualFileSystem.cpp
@@ -56,7 +56,6 @@ using namespace llvm::vfs;
using llvm::sys::fs::file_t;
using llvm::sys::fs::file_status;
using llvm::sys::fs::file_type;
-using llvm::sys::fs::kInvalidFile;
using llvm::sys::fs::perms;
using llvm::sys::fs::UniqueID;
@@ -198,7 +197,7 @@ class RealFile : public File {
: FD(RawFD), S(NewName, {}, {}, {}, {}, {},
llvm::sys::fs::file_type::status_error, {}),
RealName(NewRealPathName.str()) {
- assert(FD != kInvalidFile && "Invalid or inactive file descriptor");
+ assert(FD.isValid() && "Invalid or inactive file descriptor");
}
public:
@@ -219,7 +218,7 @@ class RealFile : public File {
RealFile::~RealFile() { close(); }
ErrorOr<Status> RealFile::status() {
- assert(FD != kInvalidFile && "cannot stat closed file");
+ assert(FD.isValid() && "cannot stat closed file");
if (!S.isStatusKnown()) {
file_status RealStatus;
if (std::error_code EC = sys::fs::status(FD, RealStatus))
@@ -236,14 +235,14 @@ ErrorOr<std::string> RealFile::getName() {
ErrorOr<std::unique_ptr<MemoryBuffer>>
RealFile::getBuffer(const Twine &Name, int64_t FileSize,
bool RequiresNullTerminator, bool IsVolatile) {
- assert(FD != kInvalidFile && "cannot get buffer for closed file");
+ assert(FD.isValid() && "cannot get buffer for closed file");
return MemoryBuffer::getOpenFile(FD, Name, FileSize, RequiresNullTerminator,
IsVolatile);
}
std::error_code RealFile::close() {
std::error_code EC = sys::fs::closeFile(FD);
- FD = kInvalidFile;
+ FD = file_t::Invalid;
return EC;
}
diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc
index be007b7abdb51..860256a1f865b 100644
--- a/llvm/lib/Support/Windows/Path.inc
+++ b/llvm/lib/Support/Windows/Path.inc
@@ -130,7 +130,7 @@ std::error_code widenPath(const Twine &Path8, SmallVectorImpl<wchar_t> &Path16,
namespace fs {
-const file_t kInvalidFile = INVALID_HANDLE_VALUE;
+const file_t::value_type file_t::Invalid = INVALID_HANDLE_VALUE;
std::string getMainExecutable(const char *argv0, void *MainExecAddr) {
SmallVector<wchar_t, MAX_PATH> PathName;
@@ -1397,7 +1397,7 @@ std::error_code unlockFile(int FD) {
std::error_code closeFile(file_t &F) {
file_t TmpF = F;
- F = kInvalidFile;
+ F = file_t::Invalid;
if (!::CloseHandle(TmpF))
return mapWindowsError(::GetLastError());
return std::error_code();
diff --git a/llvm/unittests/Support/MemoryBufferTest.cpp b/llvm/unittests/Support/MemoryBufferTest.cpp
index 2cc20c0ab5f0c..b52b4cabe2488 100644
--- a/llvm/unittests/Support/MemoryBufferTest.cpp
+++ b/llvm/unittests/Support/MemoryBufferTest.cpp
@@ -170,16 +170,22 @@ TEST_F(MemoryBufferTest, copy) {
#if LLVM_ENABLE_THREADS
TEST_F(MemoryBufferTest, createFromPipe) {
- sys::fs::file_t pipes[2];
+ int pipes[2];
#if LLVM_ON_UNIX
ASSERT_EQ(::pipe(pipes), 0) << strerror(errno);
#else
ASSERT_TRUE(::CreatePipe(&pipes[0], &pipes[1], nullptr, 0))
<< ::GetLastError();
#endif
- auto ReadCloser = make_scope_exit([&] { sys::fs::closeFile(pipes[0]); });
+ auto ReadCloser = make_scope_exit([&] {
+ sys::fs::file_t F(pipes[0]);
+ sys::fs::closeFile(F);
+ });
std::thread Writer([&] {
- auto WriteCloser = make_scope_exit([&] { sys::fs::closeFile(pipes[1]); });
+ auto WriteCloser = make_scope_exit([&] {
+ sys::fs::file_t F(pipes[1]);
+ sys::fs::closeFile(F);
+ });
for (unsigned i = 0; i < 5; ++i) {
std::this_thread::sleep_for(std::chrono::milliseconds(10));
#if LLVM_ON_UNIX
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Created using spr 1.3.7
Created using spr 1.3.7
Created using spr 1.3.7
Created using spr 1.3.7
|
ping |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of using
file_tas a type alias for different types of filehandle on different platforms, make it a concrete type to catch usages
in the codebase that mixing the usage of
file_tand its underlyingtypes. NFC.