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.

Sign up for GitHub

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

Jump to bottom

Add Crystal::System::Dir #5447

Merged
merged 1 commit into from Jan 2, 2018
Merged

Add Crystal::System::Dir #5447

merged 1 commit into from Jan 2, 2018

Conversation

RX14
Copy link
Contributor

@RX14 RX14 commented Dec 24, 2017

I decided to expose Crystal::System::Dir as an Iterator(String) since I think it's the nicest yet most generic interface for what readdir actually is: an iterator.

@RX14 RX14 mentioned this pull request Dec 24, 2017
Open
22 tasks
@larubujo
Copy link
Contributor

seem dir missing finalize, maybe from before. so iterator should have finalize. so class, not struct.

@RX14
Copy link
Contributor Author

RX14 commented Dec 24, 2017

@larubujo yes, Dir is missing a def finalize, thanks for spotting.

straight-shoota
straight-shoota reviewed Dec 25, 2017
View reviewed changes
Copy link
Member

@straight-shoota straight-shoota left a 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)
Copy link
Member

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.

Copy link
Contributor Author

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) }
Copy link
Member

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?

Copy link
Contributor Author

@RX14 RX14 Dec 26, 2017 •
edited

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).

@straight-shoota straight-shoota mentioned this pull request Dec 25, 2017
Closed
lbguilherme
lbguilherme reviewed Dec 26, 2017
View reviewed changes
src/dir.cr Outdated
@@ -65,7 +60,7 @@ class Dir
end

def each_entry
EntryIterator.new(self)
@dir
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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!

@straight-shoota straight-shoota mentioned this pull request Dec 26, 2017
Merged
@RX14
Copy link
Contributor Author

RX14 commented Dec 30, 2017

Rebased to fix travis.

@RX14
Copy link
Contributor Author

RX14 commented Dec 30, 2017 •
edited

Replaced this with a more "traditional" port to Crystal::System, and I'll refactor Dir later. This is a better port than last time, somehow I gave up halfway through and left maybe 3 platform-specific methods.

@RX14
Copy link
Contributor Author

RX14 commented Dec 31, 2017

Interesting bug:

S-tring.o: In function `*String#<=><String>:Int32':
String:(.text+0x4022): undefined reference to `*Int32@Number#<=><Int32>:Int32'
String:(.text+0x4038): undefined reference to `*Int32@Number#sign:Int32'
/usr/bin/ld: /tmp/crystal/crystal-run-crystal-spec-output.tmp: No symbol version section for versioned symbol `*Int32@Number#<=><Int32>:Int32'
/usr/bin/ld: final link failed: Nonrepresentable section on output
collect2: error: ld returned 1 exit status
Error: execution of command failed with code: 1: `cc "${@}" -o '/tmp/crystal/crystal-run-crystal-spec-output.tmp'  -rdynamic  -lpcre -lgc -lpthread /mnt/src/ext/libcrystal.a -levent -lrt -ldl -L/usr/lib -L/usr/local/lib`

re running to see if it's intermittent.

@RX14
Copy link
Contributor Author

RX14 commented Jan 1, 2018

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?

@asterite
Copy link
Member

asterite commented Jan 2, 2018

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.

@RX14
Copy link
Contributor Author

RX14 commented Jan 2, 2018

@asterite this should really be another issue, but we should codegen into tempoary files $cache/temp/random.o then perform a move operation into the final destination once we know the the compilation works (i.e. linking finishes). Moving files on the same FS is an atomic operation. That fixes the case of an interrupted (ctrl-c) compilation corrupting the cache.

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.

@RX14 RX14 merged commit 8a52f7b into crystal-lang:master Jan 2, 2018
@RX14 RX14 added this to the Next milestone Jan 2, 2018
lukeasrodgers pushed a commit to lukeasrodgers/crystal that referenced this pull request Jan 7, 2018
@RX14 @lukeasrodgers
Add Crystal::System::Dir (crystal-lang#5447)
a1d211d
@Sija Sija mentioned this pull request Mar 31, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@straight-shoota straight-shoota straight-shoota left review comments

@lbguilherme lbguilherme lbguilherme left review comments

@asterite asterite asterite approved these changes

Assignees
No one assigned
Labels
kind:refactor platform:windows topic:stdlib
Projects
None yet
Milestone
0.25.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@RX14 @larubujo @asterite @straight-shoota @lbguilherme