-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add Crystal::System::Dir #5447
Add Crystal::System::Dir #5447
Conversation
seem dir missing finalize, maybe from before. so iterator should have finalize. so class, not struct. |
@larubujo yes, |
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.
EntryIterator
can be removed. ChildIterator
as well, unless it should better be kept for èach_child` (see line comment).
src/dir.cr
Outdated
unless @dir | ||
raise Errno.new("Error opening directory '#{@path}'") | ||
end | ||
@dir = Crystal::System::Dir.each(@path) |
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.
This ivar should perhaps be renamed to match the new meaning? For example @entry_iterator
.
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.
Fixed
src/dir.cr
Outdated
@@ -101,7 +96,7 @@ class Dir | |||
end | |||
|
|||
def each_child | |||
ChildIterator.new(self) | |||
@dir.reject { |x| {".", ".."}.includes?(x) } |
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'm not sure if/why this is preferable over a custom iterator class?
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.
It's easier to read, and a custom iterator would have to wrap @dir
regardless. So comparing this to a custom iterator, the only overhead would be a single function pointer call overhead. Given that every call to next
will perform a syscall (which busts cache and takes 200+ cycles!): this is a very acceptable overhead. I realise I just complained at someone for exactly this but I think this is a different enough situation (please current me if i'm wrong).
src/dir.cr
Outdated
@@ -65,7 +60,7 @@ class Dir | |||
end | |||
|
|||
def each_entry | |||
EntryIterator.new(self) | |||
@dir |
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.
each_entry
should return a new iterator each it is called, no?
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.
If you look at the behaviour of the old iterator, this wasn't how it worked. The whole Dir
class is a very poor iterator, the each_entry
iterator simply wrapped it up into a real Iterator
, but the state was still in Dir
. This, while confusing, keeps the old Dir
semantics.
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 plan to clean up Dir
after merging this PR, and make it a module with entirely class methods.
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 disagree with the old semantics, but I do agree that this PR should not change the semantics. So let it be.
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 think everyone sane disagrees with the old semantics!
2c4810d
to
c362a7f
Compare
Rebased to fix travis. |
c362a7f
to
875e758
Compare
Replaced this with a more "traditional" port to |
Interesting bug:
re running to see if it's intermittent. |
I've only seen the above error once before, and it doesn't seem to be reproducible. Perhaps a rare race condition in the cache? |
I got race conditions in the cache many times. It would be nice to have an issue to track this, with possible suggestions for solutions. |
@asterite this should really be another issue, but we should codegen into tempoary files This specific crash i've never ever seen any error message like it, and it can't be related to an interrupted compiler, so that's very strange. |
I decided to expose
Crystal::System::Dir
as anIterator(String)
since I think it's the nicest yet most generic interface for whatreaddir
actually is: an iterator.