[llvm] r217625 - Misc cleanups to the FileSytem api.
Sean Silva
chisophugis at gmail.com
Thu Sep 11 14:18:27 PDT 2014
- bool Exists;
- if (!sys::fs::exists(LockFileName.str(), Exists) && !Exists) {
+ if (sys::fs::access(LockFileName.c_str(),
sys::fs::AccessMode::Exist) ==
+ errc::no_such_file_or_directory) {
Is there a reason you didn't just use the `bool exists(const Twine &Path)`
helper here?
-- Sean Silva
On Thu, Sep 11, 2014 at 1:30 PM, Rafael Espindola <
rafael.espindola at gmail.com> wrote:
> Author: rafael
> Date: Thu Sep 11 15:30:02 2014
> New Revision: 217625
>
> URL: http://llvm.org/viewvc/llvm-project?rev=217625&view=rev
> Log:
> Misc cleanups to the FileSytem api.
>
> The main difference is the removal of
>
> std::error_code exists(const Twine &path, bool &result);
>
> It was an horribly redundant interface since a file not existing is also a
> valid
> error_code. Now we have an access function that returns just an
> error_code. This
> is the only function that has to be implemented for Unix and Windows. The
> functions can_write, exists and can_execute an now just wrappers.
>
> One still has to be very careful using these function to avoid introducing
> race conditions (Time of check to time of use).
>
> Modified:
> llvm/trunk/include/llvm/Support/FileSystem.h
> llvm/trunk/lib/Support/LockFileManager.cpp
> llvm/trunk/lib/Support/Path.cpp
> llvm/trunk/lib/Support/Unix/Path.inc
> llvm/trunk/lib/Support/Windows/Path.inc
> llvm/trunk/unittests/Support/FileOutputBufferTest.cpp
> llvm/trunk/unittests/Support/Path.cpp
>
> Modified: llvm/trunk/include/llvm/Support/FileSystem.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/FileSystem.h?rev=217625&r1=217624&r2=217625&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/Support/FileSystem.h (original)
> +++ llvm/trunk/include/llvm/Support/FileSystem.h Thu Sep 11 15:30:02 2014
> @@ -352,33 +352,37 @@ std::error_code resize_file(const Twine
> /// not.
> bool exists(file_status status);
>
> -/// @brief Does file exist?
> +/// @brief Can the file be accessed?
> ///
> /// @param path Input path.
> -/// @param result Set to true if the file represented by status exists,
> false if
> -/// it does not. Undefined otherwise.
> -/// @returns errc::success if result has been successfully set, otherwise
> a
> +/// @returns errc::success if the path can be accessed, otherwise a
> /// platform-specific error_code.
> -std::error_code exists(const Twine &path, bool &result);
> +enum class AccessMode { Exist, Write, Execute };
> +std::error_code access(const Twine &Path, AccessMode Mode);
>
> -/// @brief Simpler version of exists for clients that don't need to
> -/// differentiate between an error and false.
> -inline bool exists(const Twine &path) {
> - bool result;
> - return !exists(path, result) && result;
> +/// @brief Does file exist?
> +///
> +/// @param Path Input path.
> +/// @returns True if it exists, false otherwise.
> +inline bool exists(const Twine &Path) {
> + return !access(Path, AccessMode::Exist);
> }
>
> /// @brief Can we execute this file?
> ///
> /// @param Path Input path.
> /// @returns True if we can execute it, false otherwise.
> -bool can_execute(const Twine &Path);
> +inline bool can_execute(const Twine &Path) {
> + return !access(Path, AccessMode::Execute);
> +}
>
> /// @brief Can we write this file?
> ///
> /// @param Path Input path.
> /// @returns True if we can write to it, false otherwise.
> -bool can_write(const Twine &Path);
> +inline bool can_write(const Twine &Path) {
> + return !access(Path, AccessMode::Write);
> +}
>
> /// @brief Do file_status's represent the same thing?
> ///
>
> Modified: llvm/trunk/lib/Support/LockFileManager.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/LockFileManager.cpp?rev=217625&r1=217624&r2=217625&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Support/LockFileManager.cpp (original)
> +++ llvm/trunk/lib/Support/LockFileManager.cpp Thu Sep 11 15:30:02 2014
> @@ -204,8 +204,8 @@ LockFileManager::WaitForUnlockResult Loc
> // If the lock file is still expected to be there, check whether it
> still
> // is.
> if (!LockFileGone) {
> - bool Exists;
> - if (!sys::fs::exists(LockFileName.str(), Exists) && !Exists) {
> + if (sys::fs::access(LockFileName.c_str(),
> sys::fs::AccessMode::Exist) ==
> + errc::no_such_file_or_directory) {
> LockFileGone = true;
> LockFileJustDisappeared = true;
> }
>
> Modified: llvm/trunk/lib/Support/Path.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Path.cpp?rev=217625&r1=217624&r2=217625&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Support/Path.cpp (original)
> +++ llvm/trunk/lib/Support/Path.cpp Thu Sep 11 15:30:02 2014
> @@ -210,13 +210,13 @@ retry_random_path:
> }
>
> case FS_Name: {
> - bool Exists;
> - std::error_code EC = sys::fs::exists(ResultPath.begin(), Exists);
> + std::error_code EC =
> + sys::fs::access(ResultPath.begin(), sys::fs::AccessMode::Exist);
> + if (EC == errc::no_such_file_or_directory)
> + return std::error_code();
> if (EC)
> return EC;
> - if (Exists)
> - goto retry_random_path;
> - return std::error_code();
> + goto retry_random_path;
> }
>
> case FS_Dir: {
>
> Modified: llvm/trunk/lib/Support/Unix/Path.inc
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Unix/Path.inc?rev=217625&r1=217624&r2=217625&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Support/Unix/Path.inc (original)
> +++ llvm/trunk/lib/Support/Unix/Path.inc Thu Sep 11 15:30:02 2014
> @@ -321,38 +321,35 @@ std::error_code resize_file(const Twine
> return std::error_code();
> }
>
> -std::error_code exists(const Twine &path, bool &result) {
> - SmallString<128> path_storage;
> - StringRef p = path.toNullTerminatedStringRef(path_storage);
> -
> - if (::access(p.begin(), F_OK) == -1) {
> - if (errno != ENOENT)
> - return std::error_code(errno, std::generic_category());
> - result = false;
> - } else
> - result = true;
> -
> - return std::error_code();
> +static int convertAccessMode(AccessMode Mode) {
> + switch (Mode) {
> + case AccessMode::Exist:
> + return F_OK;
> + case AccessMode::Write:
> + return W_OK;
> + case AccessMode::Execute:
> + return R_OK | X_OK; // scripts also need R_OK.
> + }
> + llvm_unreachable("invalid enum");
> }
>
> -bool can_write(const Twine &Path) {
> +std::error_code access(const Twine &Path, AccessMode Mode) {
> SmallString<128> PathStorage;
> StringRef P = Path.toNullTerminatedStringRef(PathStorage);
> - return 0 == access(P.begin(), W_OK);
> -}
>
> -bool can_execute(const Twine &Path) {
> - SmallString<128> PathStorage;
> - StringRef P = Path.toNullTerminatedStringRef(PathStorage);
> + if (::access(P.begin(), convertAccessMode(Mode)) == -1)
> + return std::error_code(errno, std::generic_category());
> +
> + if (Mode == AccessMode::Execute) {
> + // Don't say that directories are executable.
> + struct stat buf;
> + if (0 != stat(P.begin(), &buf))
> + return errc::permission_denied;
> + if (!S_ISREG(buf.st_mode))
> + return errc::permission_denied;
> + }
>
> - if (0 != access(P.begin(), R_OK | X_OK))
> - return false;
> - struct stat buf;
> - if (0 != stat(P.begin(), &buf))
> - return false;
> - if (!S_ISREG(buf.st_mode))
> - return false;
> - return true;
> + return std::error_code();
> }
>
> bool equivalent(file_status A, file_status B) {
>
> Modified: llvm/trunk/lib/Support/Windows/Path.inc
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Windows/Path.inc?rev=217625&r1=217624&r2=217625&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Support/Windows/Path.inc (original)
> +++ llvm/trunk/lib/Support/Windows/Path.inc Thu Sep 11 15:30:02 2014
> @@ -251,49 +251,29 @@ std::error_code resize_file(const Twine
> return std::error_code(error, std::generic_category());
> }
>
> -std::error_code exists(const Twine &path, bool &result) {
> - SmallString<128> path_storage;
> - SmallVector<wchar_t, 128> path_utf16;
> +std::error_code access(const Twine &Path, AccessMode Mode) {
> + SmallString<128> PathStorage;
> + SmallVector<wchar_t, 128> PathUtf16;
>
> - if (std::error_code ec =
> - UTF8ToUTF16(path.toStringRef(path_storage), path_utf16))
> - return ec;
> + if (std::error_code EC =
> + UTF8ToUTF16(Path.toStringRef(PathStorage), PathUtf16))
> + return EC;
>
> - DWORD attributes = ::GetFileAttributesW(path_utf16.begin());
> + DWORD Attributes = ::GetFileAttributesW(PathUtf16.begin());
>
> - if (attributes == INVALID_FILE_ATTRIBUTES) {
> + if (Attributes == INVALID_FILE_ATTRIBUTES) {
> // See if the file didn't actually exist.
> DWORD LastError = ::GetLastError();
> if (LastError != ERROR_FILE_NOT_FOUND &&
> LastError != ERROR_PATH_NOT_FOUND)
> return windows_error(LastError);
> - result = false;
> - } else
> - result = true;
> - return std::error_code();
> -}
> -
> -bool can_write(const Twine &Path) {
> - // FIXME: take security attributes into account.
> - SmallString<128> PathStorage;
> - SmallVector<wchar_t, 128> PathUtf16;
> + return errc::no_such_file_or_directory;
> + }
>
> - if (UTF8ToUTF16(Path.toStringRef(PathStorage), PathUtf16))
> - return false;
> + if (Mode == AccessMode::Write && (Attributes & FILE_ATTRIBUTE_READONLY))
> + return errc::permission_denied;
>
> - DWORD Attr = ::GetFileAttributesW(PathUtf16.begin());
> - return (Attr != INVALID_FILE_ATTRIBUTES) && !(Attr &
> FILE_ATTRIBUTE_READONLY);
> -}
> -
> -bool can_execute(const Twine &Path) {
> - SmallString<128> PathStorage;
> - SmallVector<wchar_t, 128> PathUtf16;
> -
> - if (UTF8ToUTF16(Path.toStringRef(PathStorage), PathUtf16))
> - return false;
> -
> - DWORD Attr = ::GetFileAttributesW(PathUtf16.begin());
> - return Attr != INVALID_FILE_ATTRIBUTES;
> + return std::error_code();
> }
>
> bool equivalent(file_status A, file_status B) {
>
> Modified: llvm/trunk/unittests/Support/FileOutputBufferTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/FileOutputBufferTest.cpp?rev=217625&r1=217624&r2=217625&view=diff
>
> ==============================================================================
> --- llvm/trunk/unittests/Support/FileOutputBufferTest.cpp (original)
> +++ llvm/trunk/unittests/Support/FileOutputBufferTest.cpp Thu Sep 11
> 15:30:02 2014
> @@ -7,6 +7,7 @@
> //
>
> //===----------------------------------------------------------------------===//
>
> +#include "llvm/Support/Errc.h"
> #include "llvm/Support/FileOutputBuffer.h"
> #include "llvm/Support/ErrorHandling.h"
> #include "llvm/Support/FileSystem.h"
> @@ -65,9 +66,8 @@ TEST(FileOutputBuffer, Test) {
> // Do *not* commit buffer.
> }
> // Verify file does not exist (because buffer not committed).
> - bool Exists = false;
> - ASSERT_NO_ERROR(fs::exists(Twine(File2), Exists));
> - EXPECT_FALSE(Exists);
> + ASSERT_EQ(fs::access(Twine(File2), fs::AccessMode::Exist),
> + errc::no_such_file_or_directory);
> ASSERT_NO_ERROR(fs::remove(File2.str()));
>
> // TEST 3: Verify sizing down case.
>
> Modified: llvm/trunk/unittests/Support/Path.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/Path.cpp?rev=217625&r1=217624&r2=217625&view=diff
>
> ==============================================================================
> --- llvm/trunk/unittests/Support/Path.cpp (original)
> +++ llvm/trunk/unittests/Support/Path.cpp Thu Sep 11 15:30:02 2014
> @@ -361,9 +361,8 @@ TEST_F(FileSystemTest, TempFiles) {
> EXPECT_EQ(B.type(), fs::file_type::file_not_found);
>
> // Make sure Temp2 doesn't exist.
> - bool TempFileExists;
> - ASSERT_NO_ERROR(fs::exists(Twine(TempPath2), TempFileExists));
> - EXPECT_FALSE(TempFileExists);
> + ASSERT_EQ(fs::access(Twine(TempPath2), sys::fs::AccessMode::Exist),
> + errc::no_such_file_or_directory);
>
> SmallString<64> TempPath3;
> ASSERT_NO_ERROR(fs::createTemporaryFile("prefix", "", TempPath3));
> @@ -386,8 +385,8 @@ TEST_F(FileSystemTest, TempFiles) {
> ASSERT_NO_ERROR(fs::remove(Twine(TempPath2)));
>
> // Make sure Temp1 doesn't exist.
> - ASSERT_NO_ERROR(fs::exists(Twine(TempPath), TempFileExists));
> - EXPECT_FALSE(TempFileExists);
> + ASSERT_EQ(fs::access(Twine(TempPath), sys::fs::AccessMode::Exist),
> + errc::no_such_file_or_directory);
>
> #ifdef LLVM_ON_WIN32
> // Path name > 260 chars should get an error.
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140911/7afe1bba/attachment.html>
More information about the llvm-commits
mailing list