Skip to content
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

Issue 15806: DirEntry properties on Posix have different attributes than on Windows #8193

Closed

Conversation

tspike2k
Copy link

@tspike2k tspike2k commented Aug 14, 2021

This is a potential fix for Issue 15806. This issue was caused by inconsistent use of attributes for std.file.DirEntry between Posix and Windows.

To address this, properties were marked as nothrow on Posix to match their Windows equivalents. However, this meant that DirEntry unit tests needed to be edited to no longer test if certain properties would throw an exception. Additionally, since properties were tagged as nothrow, “enforce” could no longer be used and were instead replaced by assertions. I'm not sure if this is an acceptable solution to the issue.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @tspike2k! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
15806 normal DirEntry interface inconsistency

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8193"

Copy link
Member

@CyberShadow CyberShadow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's going to be that easy to pave over this platform inconsistency.

Windows gives us file attributes "for free" when iterating. On POSIX, you have to do an additional call (stat/lstat), which is expensive, and therefore needs to be done on-demand.

@@ -3932,6 +3932,8 @@ else version (Posix)
_didLStat = false;
_didStat = false;
_dTypeSet = false;

_doStat();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This completely subverts the intent of the original design and will cause a significant performance regression.

@tspike2k
Copy link
Author

tspike2k commented Aug 14, 2021

Wow, thanks for the quick review! I was afraid that it wouldn’t be so easy, but after it passed the unit tests I was hopeful. I admit, I have hardly used stat, so I’m not familiar with all it’s intricacies. Plus, I'm still trying to wrap my head around lazy evaluation.

This is my first time doing a pull request so I'm not too comfortable with the process... Should I close the pull request? Thanks again in advance!

@CyberShadow
Copy link
Member

CyberShadow commented Aug 14, 2021

but after it passed the unit tests I was hopeful.

Yeah, performance is generally tricky to test for regressions.

Should I close the pull request?

I suppose so.

I suppose it would be possible to technically address this issue by going in the opposite direction (i.e. removing pure etc. from the Windows attributes)... which would fix the inconsistency by artificially limiting the Windows implementation. But, that would be a breaking change, so is unlikely.

I admit, I have hardly used stat, so I’m not familiar with all it’s intricacies.

It's actually possible to squeeze out even more efficiency here. Some filesystems provide the file type (file/dir/link) in the directory entry without requiring a stat call. I do this in my implementation: https://github.com/CyberShadow/ae/blob/3ddbf59cf4e6bea3a59d132ad813773b1ea8dcdc/sys/file.d#L448

@tspike2k
Copy link
Author

It's actually possible to squeeze out even more efficiency here. Some filesystems provide the file type (file/dir/link) in the directory entry without requiring a stat call. I do this in my implementation: https://github.com/CyberShadow/ae/blob/3ddbf59cf4e6bea3a59d132ad813773b1ea8dcdc/sys/file.d#L448

That’s really interesting to know, and thanks for the link! I’m always on the lookout for non-trivial code written in D. Reading through things like Phobos and Mar have really helped me to learn some of the trickier part of the language.

@tspike2k tspike2k closed this Aug 14, 2021
@tspike2k tspike2k deleted the issue-15806-const-DirEntry-on-posix branch August 14, 2021 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants