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

WIP: readlink function #10571

Closed
wants to merge 1 commit into from
Closed

WIP: readlink function #10571

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 19, 2015

Necessary for #10506 and simply wraps libuv. This is my first time working against libuv and working with ccall, any feedback is very much appreciated. Also, should we export readlink? For now I only exported it from Base.FS.

@ghost
Copy link
Author

ghost commented Mar 19, 2015

Failing due to white space errors in a commit with [ci skip]. Preparing a separate PR to fix this.

@ghost
Copy link
Author

ghost commented Mar 19, 2015

CI run now pending on #10572.

@tkelman
Copy link
Contributor

tkelman commented Mar 19, 2015

I restarted the travis build for you. Though I think it might be better to add a wrapper function in C in jl_uv.c, so the Julia-side ccall version is simpler. That's the approach I took in #7590 for example.

@tkelman
Copy link
Contributor

tkelman commented Mar 19, 2015

And as far as exporting goes, considering the number of other filesystem-related functions we currently export, I think exporting readlink would be good for now. Un-exporting portions of FS might be good to do eventually, but probably best to do a bunch of them at the same time, and export this for now.

@Keno
Copy link
Member

Keno commented Mar 19, 2015

Either this or a separate wrapper in jl_uv.c is fine to me (a separate wrapper is probably slightly better since you can put the fs struct on the stack, but you're doing a system call anyway, so it doesn't matter much). Also needs docs though.

@ghost
Copy link
Author

ghost commented Apr 2, 2015

Should this be closed in favour of #10714? I am now back from a conference and would be fine with finishing this, but it seems that there are two independent efforts at this point.

@tkelman
Copy link
Contributor

tkelman commented Apr 2, 2015

Sure, though your input there would of course be welcome. It might be useful to keep your all-Julia-side implementation of readlink around for reference, since I'm not sure exactly how much of the function we will actually want to do in C vs Julia for the final version.

@tkelman tkelman closed this Apr 2, 2015
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.

2 participants