-
Notifications
You must be signed in to change notification settings - Fork 953
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed FilesystemPath::isFile return value. #491
Conversation
Thank you! LGTM. I'll just wait for TeamCity to finish all builds and then I will merge it. AppVeyor will probably fail as we have problems with timeouts in it right now but no worries. |
The AppVeyor build failure can be ignored as it timeouted (again...). We will have to do something with it. |
src/utils/filesystem_path.cpp
Outdated
@@ -141,7 +141,11 @@ class FilesystemPathImplWindows : public FilesystemPathImpl | |||
|
|||
virtual bool isFile() override | |||
{ | |||
return !isDirectory(); | |||
WIN32_FIND_DATA ffd; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of FindFirstFile()
, wouldn't it be easier and more straightforward to use just GetFileAttributes()
?
virtual bool isFile() override
{
auto attrs = GetFileAttributes(_path.c_str());
return (attrs != INVALID_FILE_ATTRIBUTES &&
(attrs & FILE_ATTRIBUTE_DIRECTORY) == 0);
}
I am not a Windows user, but GetFileAttributes()
kind of more resembles the POSIX stat()
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @s3rvac that GetFileAttributes
makes more sense here than FindFirstFile
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have implemented it as suggested and did a forced update. At first I thought that the above would incorrectly return true on INVALID_FILE_ATTRIBUTES since it is defined as -1. However, I see that the logical && and == causes it to indeed function properly.
8239679
to
ae163f3
Compare
Instances of reinterpret_cast<HANDLE>(-1) in FilesystemPathImplWindows have been replaced with INVALID_HANDLE_VALUE for readability. GetFileAttributes was utilized in place of FindFirstFile to check for file existance and file attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
Thank you for your contribution. I've merged your changes. |
Resolves #490
@metthal I went ahead and implemented your suggestions. I also split it into a separate branch and removed these changes from the other pull request as I should have done in the first place.
Regards,
Andrew Strelsky