Optimize path check to skip to if statement if result is false. #64525
Conversation
If result is false, there is no reason to run the PathInternal.IsDirectorySeparator function, since no matter what it returns, the "result = result && ..." will always yeild false, since result is false already. So the entire if statement can be skipped, if result is false, since it cannot possible change result to a true.
ghost
added
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsIf result is false, there is no reason to run the So the entire if statement can be skipped, if result is false, since it cannot possible change result to a true.
|
| bool result = Interop.Sys.LStat(fullPath, out Interop.Sys.FileStatus fileInfo) == Interop.Errors.ERROR_SUCCESS; | ||
| if (PathInternal.IsDirectorySeparator(fullPath[fullPath.Length - 1])) | ||
|
|
||
| if (result && PathInternal.IsDirectorySeparator(fullPath[fullPath.Length - 1])) |
There was a problem hiding this comment.
could you please also remove the code duplication and move this logic to Path.Exists?
Path.ExistsCore could return a value tuple (bool exists, bool isDirectory) or have an additional out bool isDirectory parameter
There was a problem hiding this comment.
I'm not sure I understand what you mean?
What would the idea of returning if the path is a directory be, since that is not being used inside the Path.Exists function?
There was a problem hiding this comment.
currently the following logic is duplicated for both Windows and Unix:
if (result && PathInternal.IsDirectorySeparator(fullPath[fullPath.Length - 1]))
{
result = isDirectory;
}this check could be moved to File.Exists (the method that calls the methods that you have modified in this PR)
There was a problem hiding this comment.
The only code that is shared is the if statement, and since the part inside the if statement is dependent on stuff defined before the if statement, I don't think it is practical to try and move it into the normal Path.Exists.
There was a problem hiding this comment.
It's not just about the if, it's also about duplicating the comments and keeping both versions in sync.
PTAL at the following commit: fd55a4d
There was a problem hiding this comment.
I see - you are indeed correct.
I have integrated that into my branch and pushed it again.
adamsitnik
left a comment
adamsitnik
left a comment
There was a problem hiding this comment.
LGTM, thank you for your contribution @GurliGebis !
|
@adamsitnik you're very welcome. Would it make sense to submit patches to cleanup coding style across the repo? (Things like if statements seems to be quiet inconsistent, with braces being used half of the times and stuff like that) |
To be honest with you I am not a big fan of refactoring PRs as it usually takes me more time as a reviewer to review them, rather than doing it myself (applying the changes can be automated, reviewing needs to examine every line). Perhaps you would be interested in taking one of our up-for-grabs issues? You can find them here https://github.com/dotnet/runtime/issues?q=is%3Aissue+is%3Aopen+label%3Aup-for-grabs These issues are something that we definitely want to fix and need help with doing that. |
|
I'll take a look and see if there is anything I can help with on there 🙂 |
2 participants
If result is false, there is no reason to run the
PathInternal.IsDirectorySeparator(...)function.No matter what it returns, the
result = result && ...will always yeild false, since result is false already.So the entire if statement can be skipped, if result is false, since it cannot possible change result to a true.