Skip to content

Conversation

@tkelman
Copy link
Contributor

@tkelman tkelman commented Jul 19, 2015

tkelman added a commit that referenced this pull request Aug 11, 2015
add xmlReadFile and parse_file with options
@tkelman tkelman merged commit 1b55588 into master Aug 11, 2015
@tkelman tkelman deleted the tk/readfile branch August 11, 2015 07:42
@timholy
Copy link
Member

timholy commented Aug 11, 2015

LGTM

Copy link
Member

Choose a reason for hiding this comment

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

second argument should be Cstring too, since it is presumably a NUL-terminated string (you aren't passing a length). And should it be encoding::AbstractString?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see, you want to be able to pass C_NULL for encoding. That seems pretty un-Julian to expose. Maybe just have a separate parse_file(filename, options) method? (Or make encoding the third option, since in Julia it is more typical to put optional arguments last.)

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 I wanted this to be much prettier I'd probably do a keyword-argument version with all the option enums spelled out. I could maybe have called this read_file to make it more clearly a direct mapping of the C API, but doesn't seem worth the trouble to have a different argument order.

Copy link
Member

Choose a reason for hiding this comment

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

The problem with not using Cstring is that you get silent truncation of filename strings with embedded NUL chars.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just encoding strings with embedded nul's. I'd rather have this API be closer to the extensively documented upstream C API, even if that means a few methods are a little unsafe. There are bigger issues to worry about here, like memory management being kind of a mess without reference counts.

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.

4 participants