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

Win: add Crystal::System::DirHandle #4903

Closed
wants to merge 5 commits into from
Closed

Conversation

txe
Copy link
Contributor

@txe txe commented Aug 30, 2017

Moved platform specific things from Dir to Crystal::System::DirHandle
You can find winapi.cr and winerror in this PR #4832

@oprypin
Copy link
Member

oprypin commented Aug 30, 2017

Alright, so in the other PR it is decided to keep Errno as is for now, so this does not need to be discussed (though it's painful to look at).

How are we feeling about efficiency of these abstractions? A class wrapping a class that ultimately just stores a pointer doesn't seem right.

if LibC.mkdir(path.check_no_null_byte, mode) == -1
raise Errno.new("Unable to create directory '#{path}'")
end
Crystal::System::DirHandle.mkdir(path, mode)
0
Copy link
Contributor

Choose a reason for hiding this comment

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

Not exactly related to this pr, but why does ˋmkdirˋ & ˋrmdirˋ returns ˋ0ˋ?

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 don't know, but in spec there is
Dir.mkdir(path, 0o700).should eq(0)

Copy link
Member

Choose a reason for hiding this comment

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

Probably they should be marked as : Nil for avoid leaking a useless return value

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'll make another PR for it

@txe
Copy link
Contributor Author

txe commented Aug 30, 2017

I suppose, DirHandle cannot be a struct because it has a state. How about to make
class Dir < Crystal::System::DirHandle?

@oprypin
Copy link
Member

oprypin commented Aug 30, 2017

No no no, a subclass cannot work, that'll leak to documentation and other such details.

You're saying it cannot be a struct (perhaps only ideologically, because technically mutable structs are fine) but I think for these internal objects we have to make an exception and think about them as an inherent part of the public objects. Make it a struct to keep efficiency at a sane level.

Though I just argued in IRC that we could easily do without Crystal::System namespace and keep the separation by files just as it is right now by reopening the types, then we wouldn't even need to discuss this.

@asterite
Copy link
Member

Why can't the methods that change between operating systems be implemented directly in Dir without the need of introducing another type?

@oprypin
Copy link
Member

oprypin commented Aug 30, 2017

@asterite I totally agree with you, but having the Crystal::System namespace was an important decision of the core team that @txe is correctly following. May be worth discussing separately?


UPDATE: Discuss Crystal::System in #4906

@txe
Copy link
Contributor Author

txe commented Aug 30, 2017

It was suggested to move platform specific code in System, #4406.
@RX14 proposed how to do it for Dir here

@RX14
Copy link
Contributor

RX14 commented Aug 30, 2017

Yes, Crystal::System looks silly here, but there are plenty of places where it doesn't look silly and it's already in use. I think we should do it this way for consistency. Explaining 2 or 3 different ways we handle platform specific code to new contributors (or even just people who want to read the stdlib) probably isn't the best of ideas.

@straight-shoota
Copy link
Member

This now superseeded by #5447

@RX14
Copy link
Contributor

RX14 commented Jan 5, 2018

#5447 was merged, closing this

@RX14 RX14 closed this Jan 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants